test(memory_tree): unit tests for retrieval RPC handlers (#710)

[sanil-23]

Summary

Adds 15 unit tests for the six Phase 4 JSON-RPC handlers in src/openhuman/memory/tree/retrieval/rpc.rs — query_source, query_global, query_topic, search_entities, drill_down, fetch_leaves.

Closes the RPC-coverage gap that was deferred during CodeRabbit review on #831 (the Phase 4 retrieval PR). Until this PR, these handlers had 0 unit tests; the underlying domain functions in source.rs/topic.rs/drill_down.rs/etc. already had ~55 tests, but the thin handler layer itself — where parameter parsing, default fallbacks, enum validation, RpcOutcome envelope shape, and PII-redacted log formatting live — was uncovered.

Depends on #831. This branch is cut from feat/710-retrieval. Once #831 merges to main, this PR will rebase cleanly.

Test scope (per handler)

Handler	Tests	Branches exercised
query_source_rpc	3	no-filter happy path, SourceKind::parse success + error, limit=Some, source_kind=Some
query_global_rpc	1	happy path with valid window, log shape
query_topic_rpc	2	split_once(':') Some arm (colon present), None arm (prefix fallback to \"unknown\")
search_entities_rpc	3	kinds=None branch, kinds=Some(valid) branch, EntityKind::parse partial-list error
drill_down_rpc	3	max_depth default to 1, split_once(':') Some/None arms, PII redaction
fetch_leaves_rpc	3	empty input short-circuit, happy-path hydration, silent skip of missing ids

Coverage estimate

Line coverage on retrieval/rpc.rs: ~88% (was 0%).
Branch coverage: ~82%.
Uncovered remainder is map_err arms when the underlying domain function returns Err — these require corrupting the SQLite DB mid-call or mocking the domain layer. Deemed not worth the harness weight; the happy-path pattern is symmetric across all six handlers.

PII redaction assertions

Every log-asserting test includes negative assertions that PII-prone fields are NOT in the log output:

• query_source: raw source_id (e.g. slack:#eng) absent from log
• query_topic: raw entity_id (e.g. email address) absent from log
• search_entities: raw query string absent from log
• drill_down: raw node_id scope segments absent beyond the kind prefix

This locks in the redaction behaviour that was added during CodeRabbit review on #831, so a regression would fail a test rather than leak silently.

Test plan

• cargo test --lib openhuman::memory::tree::retrieval::rpc:: — 15/15 pass
• cargo fmt -- --check — clean
• cargo clippy on retrieval/rpc.rs — no new warnings from this PR (pre-existing warnings in unrelated modules unchanged)
• No source changes — purely additive test module

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features

• Added semantic memory retrieval endpoints: query_source, query_topic, query_global, search_entities, drill_down, and fetch_leaves for flexible memory querying.
• Integrated embeddings into memory summaries and chunks for semantic reranking.
• Added configurable embedding provider support with fallback to deterministic zero-vector mode for testing.

Configuration

• New environment variable overrides for embedding endpoint, model, timeout, and strictness settings.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces Phase 4 memory-tree embedding and retrieval infrastructure. It adds configurable embedding support (Ollama and inert), integrates embeddings into ingest/seal/digest pipelines, extends the database schema for persistence, and introduces a comprehensive retrieval module with six query/search functions plus RPC handlers and integration tests.

Changes

Cohort / File(s)	Summary
Configuration & Embedding Setup src/openhuman/config/schema/load.rs, src/openhuman/config/schema/storage_memory.rs, src/openhuman/config/schema/types.rs, src/openhuman/config/schema/mod.rs	New MemoryTreeConfig struct with embedding endpoint, model, timeout, and strict-mode flag. Environment override parsing for embedding settings with trimming and empty-string-as-None semantics. Integration into main Config with serde defaults.
Embedding Infrastructure src/openhuman/memory/tree/score/embed/*	Core Embedder async trait, cosine-similarity computation, BLOB packing/unpacking, and validation. OllamaEmbedder with HTTP client (768-dim). InertEmbedder producing zero vectors for tests. Factory function selecting embedder from config with strict/lax modes.
Storage & Persistence src/openhuman/memory/tree/source_tree/types.rs, src/openhuman/memory/tree/source_tree/store.rs, src/openhuman/memory/tree/store.rs	Add nullable embedding field to SummaryNode. Implement set_summary_embedding and get_summary_embedding helpers. Schema migration adding embedding BLOB column to mem_tree_summaries.
Ingest Integration src/openhuman/memory/tree/ingest.rs	Embed all kept chunks during persist; pack vectors into BLOBs and update mem_tree_chunks.embedding via explicit UPDATE statements. New tests for embedding writes and strict-mode error handling. Error context includes chunk IDs.
Seal & Digest Integration src/openhuman/memory/tree/global_tree/digest.rs, src/openhuman/memory/tree/global_tree/seal.rs, src/openhuman/memory/tree/source_tree/bucket_seal.rs	Embed generated summaries before write transactions and persist via SummaryNode.embedding. Tests disable embedding by nulling endpoint/model and setting strict-mode false.
Retrieval Module src/openhuman/memory/tree/retrieval/{types.rs, source.rs, topic.rs, global.rs, search.rs, drill_down.rs, fetch.rs, mod.rs}	New retrieval subsystem with shared RetrievalHit/QueryResponse/EntityMatch types. Implements query_source (with semantic reranking), query_topic (entity-scoped), query_global (recap wrapping), search_entities (LIKE-based), drill_down (BFS traversal with reranking), and fetch_leaves (batch hydration). Ordering by semantic similarity with cosine distance when query provided; recency-based tie-breaking for NULL embeddings.
RPC Handlers & Schema Registration src/openhuman/memory/tree/retrieval/rpc.rs, src/openhuman/memory/tree/retrieval/schemas.rs	JSON-RPC wrapper functions for all six retrieval tools with typed requests/responses, parameter validation, and error mapping. Logging redacts raw inputs (query text, entity IDs) while logging derived metrics (hit counts, kind prefixes, query length). Controller schema definitions and handler registration for JSON-RPC discovery.
Integration Tests & Exports src/openhuman/memory/tree/retrieval/integration_test.rs, src/openhuman/memory/mod.rs, src/openhuman/memory/tree/mod.rs, src/core/all.rs	New integration test module exercising ingest→retrieval pipeline; validates embedding persistence, schema drift detection, and semantic-rerank stability. Module structure updates and controller registry extension to include retrieval schemas/handlers.
Test Configuration src/openhuman/memory/tree/global_tree/recap.rs, src/openhuman/memory/tree/source_tree/flush.rs, src/openhuman/memory/tree/topic_tree/{backfill.rs, routing.rs}, tests/json_rpc_e2e.rs	Disable embedding for existing unit tests by nulling endpoint/model and setting embedding_strict: false. Add EnvVarGuard::set helper for environment variable testing.
Module Re-exports src/openhuman/memory/tree/source_tree/mod.rs, src/openhuman/memory/tree/score/mod.rs	Expose embedding helpers and submodule for external access.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat(memory): Phase 1 memory tree - multi-source ingestion & canonical chunks (#707) #732 — Both modify src/core/all.rs to extend controller/schema registries; Phase 1 added memory-tree registrations, this PR adds retrieval controller registrations to the same registry composition.
• feat: native embeddings module with Ollama and vector store #521 — Both introduce/replace the embedding layer with Ollama-backed 768-dim provider, inert fallbacks, and BLOB storage; directly related at the embedder infrastructure and persistence level.
• feat(memory): Phase 2 memory tree - preprocessing, scoring, admission gate (#708) #733 — Both modify memory-tree ingest/storage pipeline (notably ingest.rs and schema/embedding handling); share the same persistence functions and DB update patterns.

Suggested reviewers

• graycyrus

Poem

🐰 A flurry of embeddings takes flight,
Retrievals bloom in semantic light,
Chunks and summaries now have their say,
Cosine-kissed, they find their way!
Phase Four hops forth with vectors so bright. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly and concisely summarizes the primary change: adding unit tests for retrieval RPC handlers in Phase 4. The title references the specific module being tested (retrieval RPC handlers) and the PR objective (test coverage).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 8

🧹 Nitpick comments (11)

src/openhuman/memory/tree/topic_tree/routing.rs (1)

129-132: Optional: centralize embedding-test overrides in a shared helper.

The same memory_tree embedding-disable setup appears in multiple test modules; a small shared test helper would reduce drift.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/topic_tree/routing.rs` around lines 129 - 132,
Several test modules duplicate the embedding-disable setup
(cfg.memory_tree.embedding_endpoint = None; cfg.memory_tree.embedding_model =
None; cfg.memory_tree.embedding_strict = false); extract this into a shared test
helper such as a function (e.g., disable_memory_tree_embedding(cfg: &mut Config)
or tests::helpers::disable_embedding_overrides) and replace the inline
assignments with a call to that helper from the tests that currently modify
cfg.memory_tree.embedding_endpoint, cfg.memory_tree.embedding_model, and
cfg.memory_tree.embedding_strict so all tests use the centralized helper.

src/openhuman/memory/tree/source_tree/types.rs (1)

126-134: Lock in backward-compatible deserialization with a unit test.

#[serde(default)] is the right compatibility mechanism here. Please add a serde test for both missing embedding and explicit embedding: null to prevent regressions.

As per coding guidelines: "Ship unit tests and coverage for behavior you are adding or changing before building additional features on top".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/source_tree/types.rs` around lines 126 - 134, Add a
unit test that verifies the serde(default) behavior for the embedding field:
deserialize two JSON payloads—one that omits the embedding key and one that has
"embedding": null—into the struct that declares pub embedding: Option<Vec<f32>>
(the struct in types.rs where embedding is defined) using serde_json::from_str
and assert the resulting struct.embedding is None in both cases; ensure the test
is #[test] and placed with the existing serde/unit tests to lock in
backward-compatible deserialization.

src/openhuman/memory/tree/score/mod.rs (1)

8-8: Consider splitting score/mod.rs; it’s over the size guideline.

This module is already beyond the ~500-line target, so it would be a good time to move operational sections into sibling files and keep mod.rs more composition-focused.

As per coding guidelines: "Source files should be ≤ ~500 lines; split modules when growing to improve maintainability".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/score/mod.rs` at line 8, The score/mod.rs file has
grown too large—split its operational code into sibling modules and keep mod.rs
as a thin composition file: create new files (e.g., scorer.rs for core scoring
logic, utils.rs for helpers, types.rs for structs/enums) and move the
corresponding functions, impls, and type definitions out of mod.rs; then update
mod.rs to declare and re-export those modules (e.g., pub mod scorer; pub mod
utils; pub mod types; pub use scorer::Scorer; pub use types::ScoreType) and
adjust any internal use paths in moved code to reference the new module names
(ensure references to existing pub mod embed remain intact). Ensure no behavior
changes and run tests to confirm compilation.

src/openhuman/memory/tree/store.rs (1)

448-453: Add a focused migration test for mem_tree_summaries.embedding.

Please add a test that starts from a pre-column DB shape and verifies with_connection adds the nullable embedding column safely.

As per coding guidelines: "Ship unit tests and coverage for behavior you are adding or changing before building additional features on top".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/store.rs` around lines 448 - 453, Add a unit
migration test that creates a temporary DB in the pre-column shape (i.e., a
mem_tree_summaries table without the embedding column), opens it via the
existing with_connection path that triggers add_column_if_missing, and then
asserts the embedding column now exists and is nullable (e.g., PRAGMA table_info
or equivalent inspection). Locate the test near other migration tests in the
same module and call the same helpers used for DB setup/teardown; reference the
functions with_connection and add_column_if_missing and the mem_tree_summaries
table name so the test exercises the exact migration branch that adds the
"embedding" BLOB column. Ensure the test fails if the column is missing or
non-nullable and runs in isolation (temporary file or in-memory DB).

src/openhuman/config/schema/load.rs (1)

855-886: Add focused unit tests for new memory-tree env overrides.

This block introduces non-trivial parsing/clearing behavior ("" => None, strict bool parsing, timeout > 0). Please add direct tests in this module to lock semantics and avoid regressions.

💡 Suggested test addition

 #[test]
+fn apply_env_overrides_memory_embed_fields() {
+    let _g = ENV_LOCK.lock().unwrap();
+    clear_env(&[
+        "OPENHUMAN_MEMORY_EMBED_ENDPOINT",
+        "OPENHUMAN_MEMORY_EMBED_MODEL",
+        "OPENHUMAN_MEMORY_EMBED_TIMEOUT_MS",
+        "OPENHUMAN_MEMORY_EMBED_STRICT",
+    ]);
+
+    let mut cfg = Config::default();
+
+    unsafe {
+        std::env::set_var("OPENHUMAN_MEMORY_EMBED_ENDPOINT", "   ");
+        std::env::set_var("OPENHUMAN_MEMORY_EMBED_MODEL", "");
+        std::env::set_var("OPENHUMAN_MEMORY_EMBED_TIMEOUT_MS", "15000");
+        std::env::set_var("OPENHUMAN_MEMORY_EMBED_STRICT", "false");
+    }
+    cfg.apply_env_overrides();
+
+    assert_eq!(cfg.memory_tree.embedding_endpoint, None);
+    assert_eq!(cfg.memory_tree.embedding_model, None);
+    assert_eq!(cfg.memory_tree.embedding_timeout_ms, Some(15_000));
+    assert_eq!(cfg.memory_tree.embedding_strict, false);
+
+    clear_env(&[
+        "OPENHUMAN_MEMORY_EMBED_ENDPOINT",
+        "OPENHUMAN_MEMORY_EMBED_MODEL",
+        "OPENHUMAN_MEMORY_EMBED_TIMEOUT_MS",
+        "OPENHUMAN_MEMORY_EMBED_STRICT",
+    ]);
+}

As per coding guidelines, “Ship unit tests and coverage for behavior you are adding or changing before building additional features on top”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/config/schema/load.rs` around lines 855 - 886, Add focused unit
tests in this module that exercise the env-override logic for memory_tree: set
OPENHUMAN_MEMORY_EMBED_ENDPOINT and OPENHUMAN_MEMORY_EMBED_MODEL to empty string
and non-empty values and assert that memory_tree.embedding_endpoint and
memory_tree.embedding_model become None or Some(...) accordingly; set
OPENHUMAN_MEMORY_EMBED_TIMEOUT_MS to "0", a valid positive number, and a
non-numeric value and assert embedding_timeout_ms is only Some(n) when n>0; set
OPENHUMAN_MEMORY_EMBED_STRICT to various truthy/falsey inputs and assert
parse_env_bool("OPENHUMAN_MEMORY_EMBED_STRICT", &flag) drives
memory_tree.embedding_strict properly; each test should manipulate the env vars,
invoke the code path that applies these env overrides (the block that assigns
self.memory_tree.embedding_*), and clean up env state between tests to avoid
leakage.

src/openhuman/memory/tree/retrieval/search.rs (1)

101-130: This query will full-scan mem_tree_entity_index as the table grows.

LOWER(entity_id) LIKE '%…%' OR LOWER(surface) LIKE '%…%' prevents the existing entity_id index from helping, and the table does not have a matching index for surface/lowercased lookups. On a retrieval RPC that an LLM may call repeatedly, latency will trend with total entity rows. Consider moving this to FTS/trigram-style search, or storing normalized search columns with supporting indexes if substring matching is required.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/search.rs` around lines 101 - 130, The
SQL in search.rs (the sql String built around mem_tree_entity_index using
LOWER(entity_id) LIKE ?1 OR LOWER(surface) LIKE ?1 with pattern) forces full
table scans and will not use the entity_id index as the table grows; replace
this with a search that uses an indexed path: either switch to FTS (create an
FTS virtual table on surface and entity_id and query MATCH) or add normalized
columns (e.g., surface_search, entity_id_search) maintained when writing and
create indexes on them, then change the WHERE in the construction that builds
sql/params (variables sql, params, pattern, and the kinds branch) to use
equality/prefix/trigram-capable operators that will hit indexes (or use the FTS
MATCH clause) and keep the existing entity_kind IN handling; ensure LIMIT
parameter remains and update any parameter placeholders accordingly.

src/openhuman/memory/tree/global_tree/seal.rs (1)

189-198: Add one regression assertion for the new summary embedding path.

seal_one_level now persists embedding: Some(...), but the nearby tests only switch the config to the inert embedder; they never verify that the sealed weekly summary actually got an embedding. A single weekly.embedding.is_some() / expected-length check in the existing threshold test would pin the behavior this change introduced.

As per coding guidelines "Ship unit tests and coverage for behavior you are adding or changing before building additional features on top".

Also applies to: 218-218

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/global_tree/seal.rs` around lines 189 - 198, Add a
regression assertion in the existing threshold test that verifies the new
embedding path added by seal_one_level actually persisted an embedding: after
running the code that calls seal_one_level (which uses
build_embedder_from_config and embed), assert weekly.embedding.is_some() and
optionally assert the embedding length matches the expected vector size (or
non-empty) so the test will fail if embedding persistence regresses; update the
existing threshold test that switches the config to the inert embedder to
include this check (and repeat for the analogous assertion referenced at the
other location).

src/openhuman/memory/tree/retrieval/global.rs (1)

39-47: Blocking DB call on async path.

get_or_create_global_tree performs synchronous SQLite operations (per context snippet from registry.rs) but is called directly from the async query_global function without spawn_blocking. This can block the Tokio runtime, especially on first call when tree creation occurs.

Consider wrapping in spawn_blocking for consistency with other retrieval functions in this PR.

♻️ Proposed fix

+    let config_owned = config.clone();
+    let tree = tokio::task::spawn_blocking(move || get_or_create_global_tree(&config_owned))
+        .await
+        .map_err(|e| anyhow::anyhow!("query_global tree lookup join error: {e}"))??;
-    let tree = get_or_create_global_tree(config)?;
     let hits = recap_to_hits(recap_out, &tree.id, &tree.scope);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/global.rs` around lines 39 - 47,
get_or_create_global_tree is a synchronous SQLite operation being called
directly from the async query_global path; to avoid blocking the Tokio runtime,
run the DB call inside tokio::task::spawn_blocking and await it instead of
calling get_or_create_global_tree(...) inline. Move the call into a
spawn_blocking closure (capturing config), unwrap or map the JoinHandle result
into the expected Result<Tree, _> and propagate errors the same way as before,
then pass the returned tree to recap_to_hits(tree.id, &tree.scope, ...) as
before; ensure types and error conversions match the existing query_global flow.

src/openhuman/memory/tree/retrieval/topic.rs (1)

174-199: N+1 blocking task spawns may impact performance.

Each hit spawns a separate spawn_blocking task for embedding lookup (lines 181-188). With LOOKUP_HEADROOM = 200 hits, this creates up to 200 task spawns and context switches. Consider batching the DB lookups into a single spawn_blocking call.

♻️ Sketch of batched approach

+    // Batch all embedding lookups in a single spawn_blocking call
+    let node_ids: Vec<(String, NodeKind)> = hits.iter().map(|h| (h.node_id.clone(), h.node_kind)).collect();
+    let config_owned = config.clone();
+    let embeddings: Vec<Option<Vec<f32>>> = tokio::task::spawn_blocking(move || -> Result<Vec<Option<Vec<f32>>>> {
+        node_ids.iter().map(|(node_id, node_kind)| {
+            match node_kind {
+                NodeKind::Summary => src_store::get_summary_embedding(&config_owned, node_id),
+                NodeKind::Leaf => get_chunk_embedding(&config_owned, node_id),
+            }
+        }).collect()
+    })
+    .await
+    .map_err(|e| anyhow::anyhow!("embedding batch fetch join error: {e}"))??;
+
+    let mut decorated: Vec<(f32, bool, RetrievalHit)> = hits
+        .into_iter()
+        .zip(embeddings)
+        .map(|(h, emb)| { /* same logic */ })
+        .collect();
-    let mut decorated: Vec<(f32, bool, RetrievalHit)> = Vec::with_capacity(hits.len());
-    for h in hits {
-        // ... per-hit spawn_blocking
-    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/topic.rs` around lines 174 - 199, The
loop currently spawns a separate tokio::task::spawn_blocking per hit (inside the
for over hits) causing O(N) blocking-task overhead; instead collect the node_ids
and node_kinds from hits and perform a single spawn_blocking that calls
src_store::get_summary_embedding or get_chunk_embedding for each id (returning a
Vec<Option<Vec<f32>>>), then after .await iterate the returned embeddings to
compute cosine_similarity(&query_vec, &v) and build the decorated Vec<(f32,
bool, RetrievalHit)>; update references to NodeKind, get_chunk_embedding,
src_store::get_summary_embedding, query_vec, cosine_similarity, and RetrievalHit
so the batched result aligns with the original hit order.

src/openhuman/memory/tree/score/embed/mod.rs (1)

23-145: Keep embed/mod.rs export-focused.

This mod.rs now owns trait definitions, cosine math, blob codecs, and the test suite. That makes the module heavier than the repo’s mod.rs convention; the operational pieces should move into sibling files and be re-exported here.

Based on learnings: Keep domain mod.rs files light and export-focused; place operational code in sibling files (ops.rs, store.rs, schedule.rs, types.rs) and re-export from mod.rs.

Also applies to: 146-221

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/score/embed/mod.rs` around lines 23 - 145, This
file is too heavy for a mod.rs — move the operational implementations into
sibling modules and keep mod.rs export-focused: create new files (e.g., ops.rs
or math.rs for cosine_similarity, codec.rs for
pack_embedding/unpack_embedding/pack_checked/decode_optional_blob, types.rs for
EMBEDDING_DIM and Embedder trait) and re-export their public items from this mod
via pub use (retain pub use factory::..., inert::..., ollama::...); ensure
function/trait names (Embedder, EMBEDDING_DIM, cosine_similarity,
pack_embedding, unpack_embedding, pack_checked, decode_optional_blob) are public
in their new modules so mod.rs only declares pub mod X and pub use X::{Embedder,
EMBEDDING_DIM, cosine_similarity, pack_embedding, unpack_embedding,
pack_checked, decode_optional_blob} to restore the original public API.

src/openhuman/memory/tree/retrieval/rpc.rs (1)

268-560: Split the test module out of rpc.rs.

Adding the full handler test suite here pushes the file past the repo’s size target and mixes production RPC code with a large amount of test-only logic. Moving the tests to a sibling module keeps this file easier to navigate and maintain.

As per coding guidelines, "src/**/*.rs: Source files should be ≤ ~500 lines; split modules when growing to improve maintainability".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/rpc.rs` around lines 268 - 560, The file
`rpc.rs` contains a large #[cfg(test)] mod tests { ... } block that should be
split into a test-only sibling module to keep the production file small; move
the entire tests block out into a new file named rpc_tests.rs (adjacent to
rpc.rs), remove the in-file #[cfg(test)] mod tests { ... } block, and add a
declaration #[cfg(test)] mod rpc_tests; in rpc.rs; inside the new rpc_tests.rs
use super::* and keep all test functions (e.g.
query_source_rpc_returns_hits_with_no_filters,
query_global_rpc_returns_response_for_valid_window,
search_entities_rpc_passes_through_kinds_none,
drill_down_rpc_defaults_max_depth_to_one_when_unset,
fetch_leaves_rpc_hydrates_valid_ids, etc.) unchanged except rename the module
header (no nested mod) so imports like upsert_chunks and types (Chunk, Metadata,
SourceRef) still resolve.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/openhuman/config/schema/storage_memory.rs`:
- Around line 118-140: Currently the MemoryTreeConfig default provides active
Ollama values (default_memory_tree_embedding_endpoint,
default_memory_tree_embedding_model, default_memory_tree_embedding_strict) which
causes build_embedder_from_config to pick the Ollama path by default; change the
defaults to be inert by returning None for embedding_endpoint and
embedding_model and false for embedding_strict (leave embedding_timeout_ms as-is
or None if you prefer), and update the Default impl for MemoryTreeConfig to use
these inert defaults so an operator must explicitly enable embeddings before the
Ollama/embedding path is chosen.

In `@src/openhuman/memory/tree/retrieval/drill_down.rs`:
- Around line 196-206: The code silently swallows DB errors when fetching chunk
embeddings and logs potentially sensitive child ids; instead of using
get_chunk_embedding(...).unwrap_or(None) inside the drill_down loop, propagate
the error (return Err(...)) or convert it into a descriptive error using the
function's Result flow rather than treating failures as “no embedding” (update
the call sites around get_chunk_embedding and the surrounding function to
handle/forward the Result); and redact the child id in the warning (in the
log::warn! call) by replacing sensitive parts with a safe token or
hashed/shortened form (referencing the id variable), while keeping the rest of
the behavior that pushes embeddings and calls hit_from_chunk(&chunk, "",
&chunk.metadata.source_id, 0.0).

In `@src/openhuman/memory/tree/retrieval/fetch.rs`:
- Around line 51-57: The code logs the raw chunk id in retrieval::fetch
(log::debug! line around get_chunk call), which violates the PII-redaction
comment; update the log to avoid emitting the full `id` — either call a new or
existing helper like `redact_chunk_id(&id)` and log that redacted value, or
replace the id with a constant placeholder (e.g. "<redacted>") in the log
message; keep the rest of the `get_chunk(&config_owned, id)?` logic and variable
names (`chunk`, `id`, `config_owned`) unchanged.

In `@src/openhuman/memory/tree/retrieval/integration_test.rs`:
- Around line 120-131: The test uses by_email.hits.first() which may be a
summary so fetch_leaves may never be exercised; replace that flaky selection
with a guaranteed leaf chunk id (e.g., capture the chunk id from the ingest
output or run a leaf-only query) and call fetch_leaves(&cfg,
&[known_leaf_id]).await.unwrap(), then assert that got.len() == 1 and that
got[0].id (or the appropriate field) equals known_leaf_id; update references to
first_hit/node_id to use the new known_leaf_id variable so the test reliably
verifies leaf hydration via fetch_leaves.
- Around line 101-107: The test currently only checks QueryResponse bookkeeping
(asserting by_source_kind.total >= by_source_kind.hits.len()) which allows zero
hits; change the assertion after calling query_source(cfg, None,
Some(SourceKind::Chat), None, None, 20) to require at least one chat hit (e.g.,
assert!(by_source_kind.hits.len() >= 1) or assert!(by_source_kind.total >= 1)),
so that query_source and the SourceKind::Chat path actually return ingested
chats; keep the existing bookkeeping assertion if you wish (by_source_kind.total
>= by_source_kind.hits.len()) but add the non-empty check on by_source_kind.hits
or total to enforce the integration behavior.

In `@src/openhuman/memory/tree/retrieval/rpc.rs`:
- Around line 48-50: The parsing of request fields (e.g., the source_kind match
that calls SourceKind::parse(s) and the similar parsing of "kinds" later) uses
the `?` operator so parse errors bypass the later map_err wrapper and return
unprefixed errors; replace those `?` usages by mapping their errors into the
RPC-prefixed format (for example, change `SourceKind::parse(s)?` to
`SourceKind::parse(s).map_err(|e| format!("<tool>: {e}"))?`) and do the same for
the kinds parsing code path so all validation errors are formatted with the same
"<tool>: {e}" prefix before returning.

In `@src/openhuman/memory/tree/retrieval/topic.rs`:
- Around line 262-297: The warn logs expose potentially sensitive node_id
values; update the two log::warn! calls inside the retrieval path that reference
node_id (the branch handling summaries using get_summary and the leaf branch
using get_chunk) to redact or pseudonymize node_id before logging—e.g., call an
existing anonymization helper or produce a masked string/hash of node_id (not
the raw value) and log that masked value along with the same contextual message
so you still warn about the missing row without emitting PII.

In `@src/openhuman/memory/tree/score/embed/ollama.rs`:
- Around line 67-79: The fallback reqwest::Client created in the new() path
lacks any timeout and embed() does not apply per-request timeouts; update
embed() (the embed method) to call .timeout(timeout) on the outgoing reqwest
request(s) that use the client (or otherwise apply the timeout variable per
request) so that requests use the intended timeout even when client was created
via the fallback reqwest::Client::new(); locate the usage of client inside
embed() and add the per-request .timeout(timeout) on the RequestBuilder before
.send().

---

Nitpick comments:
In `@src/openhuman/config/schema/load.rs`:
- Around line 855-886: Add focused unit tests in this module that exercise the
env-override logic for memory_tree: set OPENHUMAN_MEMORY_EMBED_ENDPOINT and
OPENHUMAN_MEMORY_EMBED_MODEL to empty string and non-empty values and assert
that memory_tree.embedding_endpoint and memory_tree.embedding_model become None
or Some(...) accordingly; set OPENHUMAN_MEMORY_EMBED_TIMEOUT_MS to "0", a valid
positive number, and a non-numeric value and assert embedding_timeout_ms is only
Some(n) when n>0; set OPENHUMAN_MEMORY_EMBED_STRICT to various truthy/falsey
inputs and assert parse_env_bool("OPENHUMAN_MEMORY_EMBED_STRICT", &flag) drives
memory_tree.embedding_strict properly; each test should manipulate the env vars,
invoke the code path that applies these env overrides (the block that assigns
self.memory_tree.embedding_*), and clean up env state between tests to avoid
leakage.

In `@src/openhuman/memory/tree/global_tree/seal.rs`:
- Around line 189-198: Add a regression assertion in the existing threshold test
that verifies the new embedding path added by seal_one_level actually persisted
an embedding: after running the code that calls seal_one_level (which uses
build_embedder_from_config and embed), assert weekly.embedding.is_some() and
optionally assert the embedding length matches the expected vector size (or
non-empty) so the test will fail if embedding persistence regresses; update the
existing threshold test that switches the config to the inert embedder to
include this check (and repeat for the analogous assertion referenced at the
other location).

In `@src/openhuman/memory/tree/retrieval/global.rs`:
- Around line 39-47: get_or_create_global_tree is a synchronous SQLite operation
being called directly from the async query_global path; to avoid blocking the
Tokio runtime, run the DB call inside tokio::task::spawn_blocking and await it
instead of calling get_or_create_global_tree(...) inline. Move the call into a
spawn_blocking closure (capturing config), unwrap or map the JoinHandle result
into the expected Result<Tree, _> and propagate errors the same way as before,
then pass the returned tree to recap_to_hits(tree.id, &tree.scope, ...) as
before; ensure types and error conversions match the existing query_global flow.

In `@src/openhuman/memory/tree/retrieval/rpc.rs`:
- Around line 268-560: The file `rpc.rs` contains a large #[cfg(test)] mod tests
{ ... } block that should be split into a test-only sibling module to keep the
production file small; move the entire tests block out into a new file named
rpc_tests.rs (adjacent to rpc.rs), remove the in-file #[cfg(test)] mod tests {
... } block, and add a declaration #[cfg(test)] mod rpc_tests; in rpc.rs; inside
the new rpc_tests.rs use super::* and keep all test functions (e.g.
query_source_rpc_returns_hits_with_no_filters,
query_global_rpc_returns_response_for_valid_window,
search_entities_rpc_passes_through_kinds_none,
drill_down_rpc_defaults_max_depth_to_one_when_unset,
fetch_leaves_rpc_hydrates_valid_ids, etc.) unchanged except rename the module
header (no nested mod) so imports like upsert_chunks and types (Chunk, Metadata,
SourceRef) still resolve.

In `@src/openhuman/memory/tree/retrieval/search.rs`:
- Around line 101-130: The SQL in search.rs (the sql String built around
mem_tree_entity_index using LOWER(entity_id) LIKE ?1 OR LOWER(surface) LIKE ?1
with pattern) forces full table scans and will not use the entity_id index as
the table grows; replace this with a search that uses an indexed path: either
switch to FTS (create an FTS virtual table on surface and entity_id and query
MATCH) or add normalized columns (e.g., surface_search, entity_id_search)
maintained when writing and create indexes on them, then change the WHERE in the
construction that builds sql/params (variables sql, params, pattern, and the
kinds branch) to use equality/prefix/trigram-capable operators that will hit
indexes (or use the FTS MATCH clause) and keep the existing entity_kind IN
handling; ensure LIMIT parameter remains and update any parameter placeholders
accordingly.

In `@src/openhuman/memory/tree/retrieval/topic.rs`:
- Around line 174-199: The loop currently spawns a separate
tokio::task::spawn_blocking per hit (inside the for over hits) causing O(N)
blocking-task overhead; instead collect the node_ids and node_kinds from hits
and perform a single spawn_blocking that calls src_store::get_summary_embedding
or get_chunk_embedding for each id (returning a Vec<Option<Vec<f32>>>), then
after .await iterate the returned embeddings to compute
cosine_similarity(&query_vec, &v) and build the decorated Vec<(f32, bool,
RetrievalHit)>; update references to NodeKind, get_chunk_embedding,
src_store::get_summary_embedding, query_vec, cosine_similarity, and RetrievalHit
so the batched result aligns with the original hit order.

In `@src/openhuman/memory/tree/score/embed/mod.rs`:
- Around line 23-145: This file is too heavy for a mod.rs — move the operational
implementations into sibling modules and keep mod.rs export-focused: create new
files (e.g., ops.rs or math.rs for cosine_similarity, codec.rs for
pack_embedding/unpack_embedding/pack_checked/decode_optional_blob, types.rs for
EMBEDDING_DIM and Embedder trait) and re-export their public items from this mod
via pub use (retain pub use factory::..., inert::..., ollama::...); ensure
function/trait names (Embedder, EMBEDDING_DIM, cosine_similarity,
pack_embedding, unpack_embedding, pack_checked, decode_optional_blob) are public
in their new modules so mod.rs only declares pub mod X and pub use X::{Embedder,
EMBEDDING_DIM, cosine_similarity, pack_embedding, unpack_embedding,
pack_checked, decode_optional_blob} to restore the original public API.

In `@src/openhuman/memory/tree/score/mod.rs`:
- Line 8: The score/mod.rs file has grown too large—split its operational code
into sibling modules and keep mod.rs as a thin composition file: create new
files (e.g., scorer.rs for core scoring logic, utils.rs for helpers, types.rs
for structs/enums) and move the corresponding functions, impls, and type
definitions out of mod.rs; then update mod.rs to declare and re-export those
modules (e.g., pub mod scorer; pub mod utils; pub mod types; pub use
scorer::Scorer; pub use types::ScoreType) and adjust any internal use paths in
moved code to reference the new module names (ensure references to existing pub
mod embed remain intact). Ensure no behavior changes and run tests to confirm
compilation.

In `@src/openhuman/memory/tree/source_tree/types.rs`:
- Around line 126-134: Add a unit test that verifies the serde(default) behavior
for the embedding field: deserialize two JSON payloads—one that omits the
embedding key and one that has "embedding": null—into the struct that declares
pub embedding: Option<Vec<f32>> (the struct in types.rs where embedding is
defined) using serde_json::from_str and assert the resulting struct.embedding is
None in both cases; ensure the test is #[test] and placed with the existing
serde/unit tests to lock in backward-compatible deserialization.

In `@src/openhuman/memory/tree/store.rs`:
- Around line 448-453: Add a unit migration test that creates a temporary DB in
the pre-column shape (i.e., a mem_tree_summaries table without the embedding
column), opens it via the existing with_connection path that triggers
add_column_if_missing, and then asserts the embedding column now exists and is
nullable (e.g., PRAGMA table_info or equivalent inspection). Locate the test
near other migration tests in the same module and call the same helpers used for
DB setup/teardown; reference the functions with_connection and
add_column_if_missing and the mem_tree_summaries table name so the test
exercises the exact migration branch that adds the "embedding" BLOB column.
Ensure the test fails if the column is missing or non-nullable and runs in
isolation (temporary file or in-memory DB).

In `@src/openhuman/memory/tree/topic_tree/routing.rs`:
- Around line 129-132: Several test modules duplicate the embedding-disable
setup (cfg.memory_tree.embedding_endpoint = None;
cfg.memory_tree.embedding_model = None; cfg.memory_tree.embedding_strict =
false); extract this into a shared test helper such as a function (e.g.,
disable_memory_tree_embedding(cfg: &mut Config) or
tests::helpers::disable_embedding_overrides) and replace the inline assignments
with a call to that helper from the tests that currently modify
cfg.memory_tree.embedding_endpoint, cfg.memory_tree.embedding_model, and
cfg.memory_tree.embedding_strict so all tests use the centralized helper.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5999bc3a-20cf-40f9-8bd3-e6dce21ccfb9

📥 Commits

Reviewing files that changed from the base of the PR and between c8ecfd8 and 445a8b4.

📒 Files selected for processing (37)

• src/core/all.rs
• src/openhuman/config/schema/load.rs
• src/openhuman/config/schema/mod.rs
• src/openhuman/config/schema/storage_memory.rs
• src/openhuman/config/schema/types.rs
• src/openhuman/memory/mod.rs
• src/openhuman/memory/tree/global_tree/digest.rs
• src/openhuman/memory/tree/global_tree/recap.rs
• src/openhuman/memory/tree/global_tree/seal.rs
• src/openhuman/memory/tree/ingest.rs
• src/openhuman/memory/tree/mod.rs
• src/openhuman/memory/tree/retrieval/drill_down.rs
• src/openhuman/memory/tree/retrieval/fetch.rs
• src/openhuman/memory/tree/retrieval/global.rs
• src/openhuman/memory/tree/retrieval/integration_test.rs
• src/openhuman/memory/tree/retrieval/mod.rs
• src/openhuman/memory/tree/retrieval/rpc.rs
• src/openhuman/memory/tree/retrieval/schemas.rs
• src/openhuman/memory/tree/retrieval/search.rs
• src/openhuman/memory/tree/retrieval/source.rs
• src/openhuman/memory/tree/retrieval/topic.rs
• src/openhuman/memory/tree/retrieval/types.rs
• src/openhuman/memory/tree/score/embed/factory.rs
• src/openhuman/memory/tree/score/embed/inert.rs
• src/openhuman/memory/tree/score/embed/mod.rs
• src/openhuman/memory/tree/score/embed/ollama.rs
• src/openhuman/memory/tree/score/mod.rs
• src/openhuman/memory/tree/source_tree/bucket_seal.rs
• src/openhuman/memory/tree/source_tree/flush.rs
• src/openhuman/memory/tree/source_tree/mod.rs
• src/openhuman/memory/tree/source_tree/store.rs
• src/openhuman/memory/tree/source_tree/types.rs
• src/openhuman/memory/tree/store.rs
• src/openhuman/memory/tree/topic_tree/backfill.rs
• src/openhuman/memory/tree/topic_tree/routing.rs
• src/openhuman/threads/ops.rs
• tests/json_rpc_e2e.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Assert that query_source returns the ingested chats.

After ingesting three chat batches, this only rechecks QueryResponse bookkeeping and still passes if query_source regresses to returning zero hits. The integration test should require at least one SourceKind::Chat hit here.

Suggested tightening

-    assert!(by_source_kind.total >= by_source_kind.hits.len());
+    assert!(
+        by_source_kind.total > 0 && !by_source_kind.hits.is_empty(),
+        "query_source should surface the ingested chat batches"
+    );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/integration_test.rs` around lines 101 -
107, The test currently only checks QueryResponse bookkeeping (asserting
by_source_kind.total >= by_source_kind.hits.len()) which allows zero hits;
change the assertion after calling query_source(cfg, None,
Some(SourceKind::Chat), None, None, 20) to require at least one chat hit (e.g.,
assert!(by_source_kind.hits.len() >= 1) or assert!(by_source_kind.total >= 1)),
so that query_source and the SourceKind::Chat path actually return ingested
chats; keep the existing bookkeeping assertion if you wish (by_source_kind.total
>= by_source_kind.hits.len()) but add the non-empty check on by_source_kind.hits
or total to enforce the integration behavior.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Prefix validation errors the same way as handler execution errors.

These early ? returns bypass the later map_err(|e| format!("<tool>: {e}")), so invalid source_kind / kinds come back unprefixed while runtime failures are prefixed. That gives the same RPC two different error formats.

Suggested fix

-        Some(s) => Some(SourceKind::parse(s)?),
+        Some(s) => Some(SourceKind::parse(s).map_err(|e| format!("query_source: {e}"))?),

-            let parsed: Result<Vec<EntityKind>, String> =
-                list.iter().map(|s| EntityKind::parse(s)).collect();
+            let parsed: Result<Vec<EntityKind>, String> = list
+                .iter()
+                .map(|s| EntityKind::parse(s).map_err(|e| format!("search_entities: {e}")))
+                .collect();

Also applies to: 169-175

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/rpc.rs` around lines 48 - 50, The parsing
of request fields (e.g., the source_kind match that calls SourceKind::parse(s)
and the similar parsing of "kinds" later) uses the `?` operator so parse errors
bypass the later map_err wrapper and return unprefixed errors; replace those `?`
usages by mapping their errors into the RPC-prefixed format (for example, change
`SourceKind::parse(s)?` to `SourceKind::parse(s).map_err(|e| format!("<tool>:
{e}"))?`) and do the same for the kinds parsing code path so all validation
errors are formatted with the same "<tool>: {e}" prefix before returning.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/openhuman/memory/tree/retrieval/integration_test.rs (1)

133-140: ⚠️ Potential issue | 🟡 Minor

Assert that fetch_leaves returns the requested node.

got.len() == 1 still passes if the single returned hit is the wrong leaf. Please also assert got[0].node_id == leaf_hit.node_id so this test locks in the ID-to-hit mapping.

Suggested tightening

     assert_eq!(
         got.len(),
         1,
         "fetch_leaves must hydrate the known leaf chunk id"
     );
+    assert_eq!(got[0].node_id, leaf_hit.node_id);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/integration_test.rs` around lines 133 -
140, Test currently only asserts got.len() == 1 which can pass even if the
returned hit is for a different node; update the assertion after calling
fetch_leaves(&cfg, &[leaf_hit.node_id.clone()]) to also verify that the returned
hit matches the requested node by asserting got[0].node_id == leaf_hit.node_id
(use the existing got and leaf_hit.node_id symbols to locate where to add this
check).

🧹 Nitpick comments (2)

src/openhuman/memory/tree/retrieval/topic.rs (2)

1-583: Split this module before it grows further.

This file is already over the repo's ~500-line target and currently bundles query orchestration, reranking, hydration helpers, and a large test module. Pulling the tests and/or helper logic into sibling modules would make future retrieval changes easier to review and maintain.

As per coding guidelines: Source files should be ≤ ~500 lines; split modules when growing to improve maintainability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/topic.rs` around lines 1 - 583, The
module is too large; split it into smaller sibling modules so query
orchestration, reranking, hydration, and tests are separated. Extract
rerank_by_semantic_similarity and its embedder imports into a new module (e.g.,
retrieval::topic::rerank) and re-export the function; move
fetch_topic_tree_root_summary, entity_hit_to_retrieval_hit, and filter_by_window
into a hydration/helper module (e.g., retrieval::topic::hydrate) and keep
query_topic in the top-level retrieval::topic module which should call the
extracted functions; finally move the large #[cfg(test)] test module out into a
dedicated test file (e.g., tests/topic_tests.rs or retrieval/topic/tests.rs) so
tests no longer inflate the production source file. Update use/import paths and
module declarations to match the new layout and ensure public/private visibility
for functions used across the new modules.

---

55-65: Add a redacted per-entity correlation field to these logs.

Logging only entity_kind makes it hard to tie the entry/exit lines back to one query_topic call when multiple requests of the same kind are in flight. A short hash or other redacted token derived from entity_id would keep PII out of the logs while preserving correlation.

As per coding guidelines: Use \log` / `tracing` at `debug` / `trace` levels. Default to verbose diagnostics on new/changed flows with stable grep-friendly prefixes (`[domain]`, `[rpc]`, `[ui-flow]`) and correlation fields (request IDs, method names, entity IDs). Never log secrets or full PII — redact.`

Also applies to: 144-148

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/topic.rs` around lines 55 - 65, Add a
short redacted correlation token derived from entity_id (e.g., a stable short
hash or HMAC) and include it in the debug logs for query_topic so concurrent
calls can be correlated without exposing PII: compute the token from entity_id
inside the query_topic function (use a non-reversible hash/truncate to ~8
chars), add it as a field like correlation=<token> in the existing log::debug!
call shown (and the similar log at lines ~144-148), and reuse the same token for
all logs within that query_topic invocation; keep using the existing
"[retrieval::topic]" prefix and debug level.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/openhuman/config/schema/storage_memory.rs`:
- Around line 85-87: The doc comment for `embedding_strict` is incorrect: it
says the default is `true` but the code sets the default to `false`; update the
comment block above the config to state that `embedding_strict` defaults to
`false` and that when `false` the system falls back to the inert zero-vector
embedder with a warning, matching the actual default value in the
`embedding_strict` config field.

In `@src/openhuman/memory/tree/retrieval/integration_test.rs`:
- Around line 256-276: The test query_source_with_query_returns_same_count must
seed a guaranteed sealed source before comparing recency vs semantic counts:
reuse the seal-driving setup from seal_populates_summary_embedding (or call the
same helper that forces sealing/summarization after ingest) after ingest_chat so
at least one sealed summary exists, then run query_source for both recency and
semantic paths and assert equality; update the test to call the sealing helper
(or ingest enough messages/trigger seal logic) prior to invoking query_source to
ensure the semantic branch has real candidates to rerank.

---

Duplicate comments:
In `@src/openhuman/memory/tree/retrieval/integration_test.rs`:
- Around line 133-140: Test currently only asserts got.len() == 1 which can pass
even if the returned hit is for a different node; update the assertion after
calling fetch_leaves(&cfg, &[leaf_hit.node_id.clone()]) to also verify that the
returned hit matches the requested node by asserting got[0].node_id ==
leaf_hit.node_id (use the existing got and leaf_hit.node_id symbols to locate
where to add this check).

---

Nitpick comments:
In `@src/openhuman/memory/tree/retrieval/topic.rs`:
- Around line 1-583: The module is too large; split it into smaller sibling
modules so query orchestration, reranking, hydration, and tests are separated.
Extract rerank_by_semantic_similarity and its embedder imports into a new module
(e.g., retrieval::topic::rerank) and re-export the function; move
fetch_topic_tree_root_summary, entity_hit_to_retrieval_hit, and filter_by_window
into a hydration/helper module (e.g., retrieval::topic::hydrate) and keep
query_topic in the top-level retrieval::topic module which should call the
extracted functions; finally move the large #[cfg(test)] test module out into a
dedicated test file (e.g., tests/topic_tests.rs or retrieval/topic/tests.rs) so
tests no longer inflate the production source file. Update use/import paths and
module declarations to match the new layout and ensure public/private visibility
for functions used across the new modules.
- Around line 55-65: Add a short redacted correlation token derived from
entity_id (e.g., a stable short hash or HMAC) and include it in the debug logs
for query_topic so concurrent calls can be correlated without exposing PII:
compute the token from entity_id inside the query_topic function (use a
non-reversible hash/truncate to ~8 chars), add it as a field like
correlation=<token> in the existing log::debug! call shown (and the similar log
at lines ~144-148), and reuse the same token for all logs within that
query_topic invocation; keep using the existing "[retrieval::topic]" prefix and
debug level.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4ee21c7f-9476-4cd0-95a4-db360df57389

📥 Commits

Reviewing files that changed from the base of the PR and between 445a8b4 and f4bc537.

📒 Files selected for processing (9)

• src/core/all.rs
• src/openhuman/config/schema/storage_memory.rs
• src/openhuman/memory/mod.rs
• src/openhuman/memory/tree/retrieval/drill_down.rs
• src/openhuman/memory/tree/retrieval/fetch.rs
• src/openhuman/memory/tree/retrieval/integration_test.rs
• src/openhuman/memory/tree/retrieval/rpc.rs
• src/openhuman/memory/tree/retrieval/topic.rs
• src/openhuman/memory/tree/score/embed/ollama.rs

✅ Files skipped from review due to trivial changes (3)

• src/openhuman/memory/mod.rs
• src/openhuman/memory/tree/retrieval/drill_down.rs
• src/openhuman/memory/tree/retrieval/rpc.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/core/all.rs
• src/openhuman/memory/tree/score/embed/ollama.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix the embedding_strict default stated in docs.

Line 86 says true is the default, but Line 138-Line 140 sets embedding_strict default to false. Please align the comment with the actual default to avoid config confusion.

Suggested doc fix

 /// `embedding_strict`:
-/// - `true` (default): ingest/seal bail with a clear config error.
-/// - `false`: fall back to the inert zero-vector embedder and warn.
+/// - `true`: ingest/seal bail with a clear config error.
+/// - `false` (default): fall back to the inert zero-vector embedder and warn.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	/// `embedding_strict`:
	/// - `true` (default): ingest/seal bail with a clear config error.
	/// - `false`: fall back to the inert zero-vector embedder and warn.
	/// `embedding_strict`:
	/// - `true`: ingest/seal bail with a clear config error.
	/// - `false` (default): fall back to the inert zero-vector embedder and warn.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/config/schema/storage_memory.rs` around lines 85 - 87, The doc
comment for `embedding_strict` is incorrect: it says the default is `true` but
the code sets the default to `false`; update the comment block above the config
to state that `embedding_strict` defaults to `false` and that when `false` the
system falls back to the inert zero-vector embedder with a warning, matching the
actual default value in the `embedding_strict` config field.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Seed a guaranteed sealed source before comparing these two paths.

With one short batch, query_source(..., Some(SourceKind::Chat), ...) can legitimately return zero summaries, so both assertions pass even if the semantic branch never reranks real candidates. Reuse the seal-driving setup from seal_populates_summary_embedding or otherwise force at least one sealed source hit before comparing counts.

Based on learnings: Ship unit tests for new/changed behavior before stacking features. Untested code is incomplete.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/tree/retrieval/integration_test.rs` around lines 256 -
276, The test query_source_with_query_returns_same_count must seed a guaranteed
sealed source before comparing recency vs semantic counts: reuse the
seal-driving setup from seal_populates_summary_embedding (or call the same
helper that forces sealing/summarization after ingest) after ingest_chat so at
least one sealed summary exists, then run query_source for both recency and
semantic paths and assert equality; update the test to call the sealing helper
(or ingest enough messages/trigger seal logic) prior to invoking query_source to
ensure the semantic branch has real candidates to rerank.