Skip to content

test(memory): extend memory coverage across retrieval and tooling#2574

Merged
senamakel merged 145 commits into
tinyhumansai:mainfrom
senamakel:improve-memory-tree
May 24, 2026
Merged

test(memory): extend memory coverage across retrieval and tooling#2574
senamakel merged 145 commits into
tinyhumansai:mainfrom
senamakel:improve-memory-tree

Conversation

@senamakel
Copy link
Copy Markdown
Member

@senamakel senamakel commented May 24, 2026

Summary

  • add broad Rust coverage across memory* modules, especially retrieval, unified documents, STM recall, queue/runtime paths, and memory tree tool wrappers
  • extend direct tests for memory/read_rpc.rs, memory/retrieval/topic.rs, memory_store/unified/documents.rs, and memory_tree/tree_runtime/cli.rs
  • harden test expectations around CLI env-lock poisoning and SQLite preview fallback behavior so the focused memory suites pass on the merged branch state
  • repair broken links in src/openhuman/memory/README.md and apply the rustfmt cleanup needed for this long-running branch to satisfy the Rust quality gates

Problem

  • the memory surfaces in src/openhuman had many low-coverage wrappers, helper branches, and regression-prone edge paths
  • recent coverage work had accumulated across a long-running branch, but some focused suites still needed merged-state validation and a couple of test expectation fixes before the branch was shippable
  • the branch also carried Rust formatting drift and stale README links that blocked CI even after the new memory tests were in place

Solution

  • add targeted unit/integration-style Rust tests that exercise real temp-workspace behavior instead of only schema or JSON-shape checks
  • validate the newly added retrieval/topic, documents, CLI, and read-RPC suites on the current merged tree and fix the test-only expectation mismatches that surfaced
  • repair the broken memory/README.md relative links and commit the rustfmt cleanup required by the branch so the quality and formatting checks can pass

Submission Checklist

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
  • N/A: Diff coverage ≥ 80% is enforced by the dedicated coverage CI jobs for this PR; not separately recomputed locally in this shipping pass
  • N/A: Coverage matrix updated — coverage-only branch work; docs/TEST-COVERAGE-MATRIX.md was not changed in this shipping pass
  • N/A: All affected feature IDs from the matrix are listed in the PR description under ## Related — no matrix refresh or feature-ID remap was performed in this shipping pass
  • No new external network dependencies introduced (mock backend used per Testing Strategy)
  • N/A: Manual smoke checklist updated if this touches release-cut surfaces — this PR is memory/test/doc work, not a release-cut UX surface
  • N/A: Linked issue closed via Closes #NNN in the ## Related section — no single upstream issue was supplied for this shipping pass

Impact

  • affects Rust core memory surfaces and CLI-only test coverage; no intended desktop UI behavior change
  • improves regression detection for memory retrieval, document storage, STM recall, and tree-summarizer CLI paths
  • no migration or new external dependency impact intended

Related

  • Closes: N/A — no single tracked issue was supplied for this shipping pass
  • Follow-up PR(s)/TODOs:
    • refresh merged coverage numbers and continue climbing the remaining lowest memory* files toward the 90% goal
    • consider splitting or rebasing this long-running branch, which is significantly ahead of main
  • Feature IDs: N/A — coverage-oriented memory branch; no matrix feature-ID refresh in this shipping pass

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: improve-memory-tree
  • Commit SHA: 818ab5a7c

Validation Run

  • pnpm --filter openhuman-app format:check
  • pnpm typecheck
  • Focused tests:
    • CARGO_TARGET_DIR=target/goal-validate2 GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml openhuman::memory::retrieval::topic::tests:: --lib
    • CARGO_TARGET_DIR=target/goal-validate2 GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml openhuman::memory_store::unified::documents::tests:: --lib
    • CARGO_TARGET_DIR=target/goal-validate2 GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml openhuman::memory_tree::tree_runtime::cli::tests:: --lib
    • CARGO_TARGET_DIR=target/goal-validate2 GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml openhuman::memory::read_rpc::tests:: --lib
  • Rust fmt/check (if changed): cargo fmt --all --check
  • N/A: Tauri fmt/check (if changed) — no app/src-tauri source changes in this shipping pass

Validation Blocked

  • command: N/A
  • error: N/A
  • impact: N/A

Behavior Changes

  • Intended behavior change: none in product behavior; test/coverage expansion plus correction of test expectations on current memory codepaths
  • User-visible effect: none intended

Parity Contract

  • Legacy behavior preserved: yes; changes are coverage-oriented and keep runtime behavior unchanged
  • Guard/fallback/dispatch parity checks: targeted tests now cover CLI error surfaces, retrieval helper fallback behavior, read-RPC SQLite preview fallback, and unified-document namespace edge paths

Duplicate / Superseded PR Handling

  • Duplicate PR(s): none
  • Canonical PR: this PR
  • Resolution (closed/superseded/updated): updated

senamakel added 30 commits May 23, 2026 20:46
Two architectural cleanups in memory_tree:

1. Summariser slim-down (-1000 LoC)
   - Delete Summariser trait + InertSummariser + LlmSummariser. The
     soft-fallback ladder, structured-facet side-channel, and config
     resolution all collapse into one async fn summarise() in
     memory_tree/summarise.rs.
   - fallback_summary() is the deterministic concat-and-truncate
     helper for seal cascades that must not abort on LLM failure
     (replaces InertSummariser's soft-fallback role at the call site).
   - Tests inject via chat::test_override::with_provider() — a tokio
     task_local override, no global mutex, no test serialization.

2. Tree unification — one tree, not three
   - tree_source/ deleted. Generic tree mechanics (append, flush,
     cascade-seal, store, types) live in memory_tree/tree/.
   - Source-specific .md mirror + get_or_create_source_tree split out
     into memory_tree/sources/.
   - tree_global/registry.rs and tree_topic/registry.rs collapse from
     ~150 LoC of duplicated UNIQUE-race dance into 4-line wrappers
     over a new generic tree::registry::get_or_create_tree().
   - tree_global and tree_topic are now honest policy adapters (digest
     cascade; hotness curator + fanout) that import from tree::*.

No behavioral changes — same SQL schema, same RPC names, same wire
types. 698/698 memory_tree tests pass.

Net: -822 LoC across 43 files.
This commit refactors the memory management structure by moving various modules from the `memory_tree` namespace to a unified `memory` namespace. Key changes include:

- Updated imports across multiple files to reflect the new module paths.
- Consolidated functionality related to chunk storage, content reading, and job management under the `memory` namespace.
- Introduced new tests for the chunk store to ensure functionality remains intact.

No behavioral changes were introduced, and all existing tests pass successfully.
Tidy-up to land the in-progress reorg before the memory_store refactor:
- chunker stays under memory_tree (uses memory_tree::chunker)
- ingest_document tool stays under memory_tree/tools
- drop the now-empty memory/tools/mod.rs and stale ingest_chunker references
- align memory/mod.rs module list with the actual files on disk
- Moved all files from src/openhuman/memory/store/ to new top-level
  src/openhuman/memory_store/ module.
- Updated all internal imports within moved files from
  memory::store:: to memory_store::.
- Made unified and agentmemory submodules pub in memory_store/mod.rs
  so the re-export shim in memory/store/mod.rs can re-export them.
- Left src/openhuman/memory/store/mod.rs as a backwards-compat shim
  (pub use crate::openhuman::memory_store::*) so callers that still
  reference memory::store:: continue to compile.
- Wired memory_store as a new top-level pub mod in openhuman/mod.rs.
- Moved all files from src/openhuman/memory/content_store/ to new
  src/openhuman/memory_store/content/ submodule.
- Updated all internal imports within moved files from
  memory::content_store:: to memory_store::content::.
- Added pub mod content in memory_store/mod.rs.
- Replaced src/openhuman/memory/content_store/ directory with a single
  memory/content_store.rs shim that re-exports memory_store::content::*
  and all submodules for backwards-compat.
…unks/

- Created src/openhuman/memory_store/chunks/ with mod.rs, store.rs
  (from chunk_store.rs), types.rs (from chunk_types.rs), and
  store_tests.rs (from chunk_store_tests.rs).
- Updated #[path] attr in store.rs to point at store_tests.rs.
- Updated internal chunk_types:: references in store.rs and
  store_tests.rs to use memory_store::chunks::types::.
- Replaced old memory/chunk_store.rs and memory/chunk_types.rs with
  re-export shims pointing at memory_store::chunks::{store,types}.
- Added pub mod chunks in memory_store/mod.rs.
- Created src/openhuman/memory_store/vectors/ with mod.rs, store.rs
  (from embeddings/store.rs) and store_tests.rs.
- Updated super::EmbeddingProvider import to full crate path in moved
  store.rs (since super is now memory_store::vectors, not embeddings).
- Added explicit EmbeddingProvider import in store_tests.rs.
- Replaced embeddings/store.rs with a re-export shim pointing at
  memory_store::vectors::store::*.
- Added pub mod vectors in memory_store/mod.rs.
- Created memory_store/trees/ with store.rs (from tree/store.rs),
  types.rs (from tree/types.rs), store_tests.rs, mod.rs.
  Updated tree::types:: refs to memory_store::trees::types:: in store.rs.
- Created memory_store/trees_global/ with registry.rs (from
  tree_global/registry.rs) and mod.rs.
  Note: tree_global had no standalone store.rs — it uses tree::store
  directly. No digest.rs/seal.rs moved (mix logic with persistence).
- Created memory_store/trees_topic/ with store.rs, types.rs,
  registry.rs (from tree_topic/) and mod.rs.
  Updated tree_topic::types:: refs to memory_store::trees_topic::types::.
- Added pub mod trees/trees_global/trees_topic in memory_store/mod.rs.
- Replaced all original files with re-export shims pointing at
  memory_store::trees*/. Skipped: tree/bucket_seal.rs, tree/flush.rs,
  tree/registry.rs, tree_global/digest.rs, tree_global/seal.rs,
  tree_global/recap.rs, tree_topic/backfill.rs, tree_topic/curator.rs,
  tree_topic/hotness.rs, tree_topic/routing.rs — all mix logic with
  persistence.
Drop the transitional shims left by steps 1-5 and update every remaining
caller to import from `memory_store::*` directly:
- Delete memory/store/, memory/content_store.rs, memory/chunk_store{,_tests}.rs,
  memory/chunk_types.rs, embeddings/store{,_tests}.rs, and the three
  memory_tree/tree*/{store,types,registry}.rs shim files.
- Repoint ~80 call sites across memory/, memory_tree/, learning/, doctor/,
  composio/, agent/harness/, context/, screen_intelligence/, subconscious/
  and the helper binaries (gmail-backfill-3d, memory-tree-init-smoke) to
  memory_store::{content,chunks,vectors,trees,trees_global,trees_topic}.
- Remove obsolete README at memory/store/README.md (content lives in
  memory_store/README.md).
…word/tag)

Single aggregation entry point over the four memory_store retrieval modes:
- tree_walk    -> memory::retrieval::drill_down (reverse-dep documented)
- vector_search/keyword_search -> UnifiedMemory::query_namespace_hits
- param_tag_search -> chunks::store::list_chunks + Metadata.tags AND-filter

Adds `ParamTagFilters` for structured chunk queries. No scoring or storage
logic moved; the facade is purely a thin import surface so callers don't
have to reach into four separate submodules.
Two additions to make memory_store the single import surface for every
stored memory kind:

- memory_store::contacts/ — thin re-export of people::store (PeopleStore
  singleton, Person/Handle/Interaction types) plus async helpers
  get_contact / list_contacts / lookup_contact that match the fail-soft
  read contract of the rest of the retrieval surface. No data is moved;
  PeopleStore remains the source of truth.

- memory_store::kinds — MemoryKind enum naming all eight persisted
  shapes (Content, Chunk, Tree, Vector, Document, Kv, Graph, Contact)
  plus a kinds::types module that aliases the canonical Rust type for
  each kind. One stop to answer "what does memory_store store?".
Make "everything in memory_store is vector and obsidian compatible" a
compiler-enforced contract instead of a slogan:

- VectorEmbeddable: every stored kind returns its MemoryKind and a
  deterministic embeddable text fed to the embedding model.
- ObsidianRepresentable: every stored kind renders an ObsidianFile
  (vault-relative path + full markdown body with YAML front-matter) that
  can be written into the content store and opened in Obsidian unmodified.

Impls cover Chunk, SummaryNode, Tree, Person (Contact), and
StoredMemoryDocument. Adding a new MemoryKind variant now requires
implementing both traits or the build breaks.
…arness

Three new tools live one layer below the curated memory_tree_* tools so an
agent can inspect or operate on raw memory_store rows directly:

- memory_store_raw_search  -> entity-index free-text search (delegates to
  memory::retrieval::search_entities).
- memory_store_raw_chunks  -> structured chunk filter by source/owner/time
  with required-tag post-filter (delegates to chunks::store::list_chunks).
- memory_store_kinds       -> introspection: enumerate every MemoryKind
  variant for fan-out planning.

All three follow the project Tool trait, return JSON, and reuse the
config_rpc::load_config_with_timeout bootstrap.
All three tree flavors (Source, Global, Topic) already shared the same
table keyed by TreeKind. The trees_global/ and trees_topic/ sibling
directories were just thin per-kind wrappers around the generic registry
plus, in trees_topic's case, an unrelated entity-hotness side-table.

Folded into trees/ so "what does memory_store know about trees?" is a
single-directory answer:
- trees/registry.rs   — kind-parameterized get_or_create_{global,topic}_tree,
  list_trees_by_kind (+ list_topic_trees alias), archive_tree (+
  archive_topic_tree alias).
- trees/hotness.rs    — mem_tree_entity_hotness persistence (was
  trees_topic/store.rs).
- trees/types.rs      — Tree/SummaryNode/TreeKind/TreeStatus plus the
  hotness types/constants (was trees_topic/types.rs).
- trees/mod.rs        — single re-export surface.

Deleted memory_store/trees_global/ and memory_store/trees_topic/. Swept
imports across memory/jobs, memory/retrieval, and memory_tree/tree*/ to
use the new memory_store::trees::{registry, hotness, types} paths. The
memory_tree/tree_topic/ module re-exports trees::hotness as `store` to
preserve the legacy `tree_topic::store::*` call shape.
…ator

Aligns the layer boundary per the new architecture:
- memory_store/  -> storage primitives (one Tree type, one schema).
- memory_tree/   -> generic tree mechanics (chunker, summarise, bucket-seal,
                    flush, walk). Kind-agnostic.
- memory/        -> orchestrator + job handler. OWNS the policy that
                    builds global and topic trees on top of generic
                    mechanics.

Moves:
- src/openhuman/memory_tree/tree_global/ -> src/openhuman/memory/tree_global/
- src/openhuman/memory_tree/tree_topic/  -> src/openhuman/memory/tree_topic/

memory_tree/mod.rs keeps `pub use crate::openhuman::memory::tree_{global,topic};`
re-exports so `memory_tree::tree_global::*` / `tree_topic::*` paths still
resolve during the migration. No call-site changes required.
Chunking is a storage primitive — the unit memory_store persists is a
chunk, so the code that produces chunks belongs next to chunks/ and
content/, not in higher-level orchestrator/tree modules.

Moves:
- src/openhuman/memory_tree/chunker.rs -> memory_store/chunker/source_kind.rs
  (Phase B source-kind dispatch chunker used by ingest_pipeline)
- src/openhuman/memory/chunker.rs      -> memory_store/chunker/semantic.rs
  (heading/paragraph-aware chunker used by UnifiedMemory writes)

memory_store/chunker/mod.rs re-exports both; `chunk_markdown` keeps
pointing at the source_kind variant (its existing call shape) and the
semantic variant is exposed as `chunk_semantic`. Updated the two
external callers (memory/ingest_pipeline.rs and
memory_store/unified/helpers.rs) and dropped the now-empty `chunker`
entries from memory/mod.rs and memory_tree/mod.rs.
Tool-scoped memory (durable learnings + high-priority rules per tool) is
a distinct storage domain — not just another namespace under the generic
memory module. Lifting it to a sibling top-level module makes that
boundary explicit and gives it room to grow its own RPC surface and
agent tools.

Moved: src/openhuman/memory/tool_memory/ -> src/openhuman/memory_tools/

memory/mod.rs keeps `pub use crate::openhuman::memory_tools as tool_memory;`
so existing call sites (memory::tool_memory::*) continue to resolve
through the alias. No behavior change.
Surface read/write into the tool-scoped memory store as first-class agent
tools so the agent can introspect rules mid-session and record new ones:

- memory_tools_list -> ToolMemoryStore::list_rules for a given tool name.
- memory_tools_put  -> upsert a ToolMemoryRule (priority + tags) under the
  UserExplicit source.

Both go through `memory::ops::helpers::active_memory_client` to reuse the
existing memory-handle bootstrap, then wrap a fresh `ToolMemoryStore`.
Drops the optional REST-server backend that proxied memory ops to an
external `agentmemory` daemon. The unified SQLite + vector path is the
only supported memory backend.

- Deleted src/openhuman/memory_store/agentmemory/ (client, backend,
  mapping, mod, README).
- Removed the AGENTMEMORY_BACKEND constant, is_agentmemory_backend helper,
  and the short-circuit branch in factories::create_memory_full.
- Dropped the pub mod + re-exports from memory_store/mod.rs.

The agentmemory_url / agentmemory_secret / agentmemory_timeout_ms config
fields and the about_app catalog entry are descriptive metadata only and
left in place; they no longer wire anything.
Defines the contract types callers above memory_tree (orchestrator, jobs,
RPC) use to write into and read from any tree. One pair per direction so
the boundary shape is uniform regardless of tree kind.

Write:
- TreeLeafPayload    -- serde mirror of LeafRef
- TreeLabelStrategy  -- serde mirror of bucket_seal::LabelStrategy
- TreeWriteRequest   -- target tree_id/kind + leaf + strategy + deferred flag
- TreeWriteOutcome   -- new_summary_ids from any seal cascade + seal_pending

Read:
- TreeReadRequest    -- tree_id + start_node_id + max_depth + optional query
                        for vector rerank + limit
- TreeReadHit        -- compact projection (node_id, node_kind, level,
                        content, score)
- TreeReadResult     -- hits + total + echoed tree_id

Pure contract types: no logic, no IO. From/Into conversions hand off to
the existing LeafRef primitive. Re-exported from memory_tree::mod.
Both top-level READMEs were materially out of date:

- memory_tree/README.md still listed canonicalize/, content_store/,
  jobs/, retrieval/, score/, tree_source/, tree_topic/, tree_global/,
  util/, store.rs, store_tests.rs, ingest.rs, rpc.rs, types.rs, and
  schemas.rs - none of which live in memory_tree anymore. Replaced
  with the current minimal surface (io, tree/, summarise, sources/,
  tools/) and an explicit layer-rules section.

- memory_store/README.md still described it as just the SQLite +
  UnifiedMemory housing. Replaced with a tour of the eight storage
  submodules (chunker/chunks/content/trees/vectors/unified/contacts +
  kinds/traits) and the new layer rules (content bytes immutable,
  SQLite for indexing + vectors, no upward deps).

Also added the previously-undocumented raw.rs + obsidian.rs entries to
memory_store/content/README.md.
Missed in the prior README sweep — the two files existed but had no entry.
create_memory_with_storage and create_memory_with_storage_and_routes both
had zero external callers — they were either dead (no users at all) or
only called from create_memory itself. Inlined create_memory's call to
go straight to create_memory_full, deleted both wrappers, updated the
re-export lists in memory_store/mod.rs + memory/mod.rs, and refreshed
the doc references in client.rs.

Public factory surface is now three functions:
- create_memory             - default config
- create_memory_with_local_ai - per-workload local embedding model
- create_memory_for_migration - migration-specific bootstrap
…ngs_probed

Function still exists (used by factories' own tests) but had zero external
consumers. Removed from memory_store/mod.rs and memory/mod.rs re-export
lists so the public surface only advertises what's actually used.
Audit found memory_tools is well-sized as-is (no dead exports, no
bloated files — every public symbol has external callers). The only
gaps were missing surface docs.

- Added memory_tools/README.md mirroring the layout used by
  memory_store/README.md and memory_tree/README.md (file table + how-it-
  fits + layer rules).
- Updated mod.rs banner to list the new tools/ submodule
  (MemoryToolsListTool / MemoryToolsPutTool) alongside the existing
  types/store/capture/prompt components.
chunker/ produced what chunks/ stored — same domain, two directories.
Consolidating into one module so the full chunk lifecycle (produce,
type, persist) lives together.

- src/openhuman/memory_store/chunker/semantic.rs   -> chunks/semantic.rs
- src/openhuman/memory_store/chunker/source_kind.rs -> chunks/produce.rs
  (renamed: "produce" reads better next to "store" than "source_kind")
- chunker/mod.rs deleted; its re-exports folded into chunks/mod.rs.

Updated the two callers (memory/ingest_pipeline.rs and
memory_store/unified/helpers.rs) and the memory_store README to reflect
the single chunks/ directory.
…tore

First step toward retiring memory_store/unified/. New top-level
memory_archivist captures conversation turns to disk as Obsidian-
compatible markdown:

  <content_root>/episodic/<session_id>/<seq:06>.md

YAML front-matter carries session_id / seq / timestamp_ms / role /
cost_microdollars / lesson / tool_calls_json; body is the turn text.
Immutable after first write (same contract as content::atomic).

API surface mirrors the unified::fts5 episodic functions the archivist
and STM recall use today:
- record_turn(config, ArchivedTurn) -> ArchivedTurn
  (replaces fts5::episodic_insert)
- session_entries(config, session_id) -> Vec<ArchivedTurn>
  (replaces fts5::episodic_session_entries)

No SQLite, no async, no upward deps — only borrows write_if_new from
memory_store::content::atomic. Cross-session search intentionally not
ported; it's the right job for the upcoming grep tools.

Wired into src/openhuman/mod.rs alongside the other memory_* modules
and includes a README + 5 unit tests covering round-trip, append,
missing sessions, lesson/tool_calls preservation, and session isolation.
The harness + STM recall continue to use unified::fts5 until a follow-up
migrates them.
Second step toward retiring memory_store/unified/. Replaces unified::graph
with a read-only module that derives entity edges from the existing
mem_tree_entity_index — no parallel triple-store table, no new schema.

Premise: the graph IS the tree mapped out. Two entities that co-occur on
the same node form an edge; weight is the count of distinct shared nodes.

API:
- co_occurring_entities(config, subject, limit) -> Vec<GraphEdge>
  (SELF-JOIN on mem_tree_entity_index, ordered by weight DESC)
- neighbors(config, subject, limit) -> Vec<String> (ids only)
- query::group_by_weight(edges) -> HashMap<weight, Vec<id>>

Excludes self-edges. Intentionally does NOT cover the LLM-extracted
(subject, predicate, object) triples that ingestion currently writes via
unified::graph::graph_upsert_namespace — that surface needs its own
decision (drop or persist as md).

Wired into src/openhuman/mod.rs. Five unit tests cover empty result,
single co-occurrence, weight-counts-distinct-nodes, self-edge exclusion,
and weight-ordered neighbors.
…ector/kv/contacts

Per the new direction (memory_store holds raw + entities + tree + vector
+ kv + contacts; everything else stores into it):

- Lifted unified/kv.rs -> memory_store/kv.rs as a peer of trees/vectors.
  The impl UnifiedMemory block stays in the file; once the Memory trait
  callers migrate to per-kind backends it'll split into free functions.
- Added memory_store/entities/ — thin re-export of memory::score::store
  for the mem_tree_entity_index table so callers reach for entities at
  memory_store::entities::* rather than crossing into the score module.
- MemoryKind enum reshaped to the new catalog: Raw, Chunk, Entity,
  Tree, Vector, Kv, Contact. Dropped Document (md is the backend now)
  and Graph (memory_graph derives edges, no first-class storage kind).
- Removed the StoredMemoryDocument trait impls in memory_store/traits.rs
  for the same reason — documents map to raw md files.
- README updated to lead with the canonical kind list and to flag
  unified/ as staging for removal.
New top-level module storing people and other named entities as
Obsidian-compatible markdown under <content_root>/entities/<kind>/.
Replacement scaffold for the SQLite-backed people/ — both run in
parallel until callers migrate.

Per-file YAML front-matter carries id / kind / display_name / aliases /
emails / handles / created_at / updated_at; body is free-form notes
preserved across upserts so the user can hand-edit in Obsidian without
losing changes on re-write.

API:
- put_entity(config, Entity) -> Entity   (atomic upsert, notes-preserving)
- get_entity(config, kind, id)           -> Option<Entity>
- list_entities(config, kind)            -> Vec<Entity>
- lookup_alias(config, kind, needle)     -> Option<Entity>
  (case-insensitive linear scan over aliases/emails/handles/display_name)

EntityKind mirrors memory::score::extract::EntityKind so canonical ids
the scorer emits round-trip unchanged.

Six unit tests cover round-trip persistence, missing-entity = None,
list-by-kind isolation, the four-way alias lookup, and notes-body
preservation across upserts.

Drive-by: swept the test-only `use crate::openhuman::memory::content_store`
imports across memory/retrieval/, memory/tree_global/, and memory_tree/tree/
to the current `memory_store::content as content_store` alias so cargo
test compiles again.
@senamakel senamakel requested a review from a team May 24, 2026 18:31
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 24, 2026

Warning

Review limit reached

@senamakel, we couldn't start this review because you've used your available PR reviews for now.

Your plan includes 5 reviews of capacity. Refill in 48 minutes and 2 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1167c5f9-3c7b-4158-a339-f5d414fb61f3

📥 Commits

Reviewing files that changed from the base of the PR and between 1c8b172 and b8f2be7.

📒 Files selected for processing (106)
  • src/bin/gmail_backfill_3d.rs
  • src/bin/slack_backfill.rs
  • src/core/cli.rs
  • src/openhuman/agent/harness/archivist.rs
  • src/openhuman/agent/harness/archivist_tests.rs
  • src/openhuman/composio/ops_test.rs
  • src/openhuman/composio/providers/gmail/ingest.rs
  • src/openhuman/composio/providers/slack/ingest.rs
  • src/openhuman/context/segment_recap_summarizer_tests.rs
  • src/openhuman/embeddings/mod.rs
  • src/openhuman/memory/README.md
  • src/openhuman/memory/ingest_pipeline.rs
  • src/openhuman/memory/ingestion/mod.rs
  • src/openhuman/memory/ingestion/parse.rs
  • src/openhuman/memory/ingestion/rules.rs
  • src/openhuman/memory/mod.rs
  • src/openhuman/memory/ops/documents.rs
  • src/openhuman/memory/ops/helpers.rs
  • src/openhuman/memory/ops/kv_graph.rs
  • src/openhuman/memory/ops/learn.rs
  • src/openhuman/memory/ops/sync.rs
  • src/openhuman/memory/ops/tool_memory.rs
  • src/openhuman/memory/ops_tests.rs
  • src/openhuman/memory/read_rpc.rs
  • src/openhuman/memory/retrieval/benchmarks.rs
  • src/openhuman/memory/retrieval/drill_down.rs
  • src/openhuman/memory/retrieval/fetch.rs
  • src/openhuman/memory/retrieval/global.rs
  • src/openhuman/memory/retrieval/integration_test.rs
  • src/openhuman/memory/retrieval/rpc.rs
  • src/openhuman/memory/retrieval/schemas.rs
  • src/openhuman/memory/retrieval/search.rs
  • src/openhuman/memory/retrieval/source.rs
  • src/openhuman/memory/retrieval/types.rs
  • src/openhuman/memory/schemas/documents.rs
  • src/openhuman/memory/schemas/files.rs
  • src/openhuman/memory/schemas/kv_graph.rs
  • src/openhuman/memory/schemas/tool_memory.rs
  • src/openhuman/memory/score/signals/ops.rs
  • src/openhuman/memory/score/store.rs
  • src/openhuman/memory/stm_recall/recall_tests.rs
  • src/openhuman/memory/tree_rpc.rs
  • src/openhuman/memory_archivist/clip.rs
  • src/openhuman/memory_archivist/compose.rs
  • src/openhuman/memory_archivist/store.rs
  • src/openhuman/memory_archivist/tree_writer.rs
  • src/openhuman/memory_entities/store.rs
  • src/openhuman/memory_graph/query.rs
  • src/openhuman/memory_queue/handlers/mod.rs
  • src/openhuman/memory_queue/scheduler.rs
  • src/openhuman/memory_queue/store.rs
  • src/openhuman/memory_queue/types.rs
  • src/openhuman/memory_queue/worker.rs
  • src/openhuman/memory_store/chunks/produce.rs
  • src/openhuman/memory_store/chunks/store.rs
  • src/openhuman/memory_store/contacts/mod.rs
  • src/openhuman/memory_store/content/read.rs
  • src/openhuman/memory_store/content/tags.rs
  • src/openhuman/memory_store/factories.rs
  • src/openhuman/memory_store/kv.rs
  • src/openhuman/memory_store/memory_trait.rs
  • src/openhuman/memory_store/mod.rs
  • src/openhuman/memory_store/retrieval/mod.rs
  • src/openhuman/memory_store/tools/kinds.rs
  • src/openhuman/memory_store/tools/raw_chunks.rs
  • src/openhuman/memory_store/tools/raw_search.rs
  • src/openhuman/memory_store/traits.rs
  • src/openhuman/memory_store/trees/registry.rs
  • src/openhuman/memory_store/trees/store.rs
  • src/openhuman/memory_store/unified/graph.rs
  • src/openhuman/memory_store/unified/init.rs
  • src/openhuman/memory_tools/store.rs
  • src/openhuman/memory_tools/store_tests.rs
  • src/openhuman/memory_tools/test_helpers.rs
  • src/openhuman/memory_tools/tools/list.rs
  • src/openhuman/memory_tools/tools/put.rs
  • src/openhuman/memory_tree/global/digest.rs
  • src/openhuman/memory_tree/global/digest_tests.rs
  • src/openhuman/memory_tree/global/mod.rs
  • src/openhuman/memory_tree/global/recap.rs
  • src/openhuman/memory_tree/global/seal.rs
  • src/openhuman/memory_tree/io.rs
  • src/openhuman/memory_tree/mod.rs
  • src/openhuman/memory_tree/sources/registry.rs
  • src/openhuman/memory_tree/summarise.rs
  • src/openhuman/memory_tree/tools/drill_down.rs
  • src/openhuman/memory_tree/tools/fetch_leaves.rs
  • src/openhuman/memory_tree/tools/ingest_document.rs
  • src/openhuman/memory_tree/tools/mod.rs
  • src/openhuman/memory_tree/tools/query_global.rs
  • src/openhuman/memory_tree/tools/query_source.rs
  • src/openhuman/memory_tree/tools/query_topic.rs
  • src/openhuman/memory_tree/tools/search_entities.rs
  • src/openhuman/memory_tree/tools/walk.rs
  • src/openhuman/memory_tree/topic/backfill.rs
  • src/openhuman/memory_tree/topic/curator.rs
  • src/openhuman/memory_tree/topic/mod.rs
  • src/openhuman/memory_tree/topic/routing.rs
  • src/openhuman/memory_tree/tree/bucket_seal.rs
  • src/openhuman/memory_tree/tree/bucket_seal_tests.rs
  • src/openhuman/memory_tree/tree/flush.rs
  • src/openhuman/memory_tree/tree/mod.rs
  • src/openhuman/memory_tree/tree/registry.rs
  • src/openhuman/memory_tree/tree_runtime/engine.rs
  • src/openhuman/memory_tree/tree_runtime/ops.rs
  • src/openhuman/memory_tree/tree_runtime/schemas.rs
📝 Walkthrough

Walkthrough

This PR reorganizes memory-related modules, adds new archivist, entity, graph, sync, queue, and tool surfaces, removes legacy agentmemory and memory_tree chat layers, rewires runtime and retrieval call sites to new module paths, expands tests and docs, and updates the e2e smoke test to manage a mock server lifecycle.

Changes

Memory architecture refactor

Layer / File(s) Summary
Public module surfaces and contracts
src/openhuman/memory/*, src/openhuman/memory_store/*, src/openhuman/memory_sync/*, src/openhuman/memory_tree/*, src/openhuman/memory_conversations/*, src/openhuman/memory_tools/*
Adds new memory chat/runtime contracts, tree IO and summarization contracts, sync traits, storage catalogs, vector/traits surfaces, tool module exports, and conversation type relocation while trimming legacy re-exports.
Archivist dual-write and session migration
src/openhuman/agent/harness/archivist.rs, src/openhuman/memory_archivist/*, src/openhuman/memory_store/unified/segments*.rs, src/openhuman/memory_conversations/*
Archivist now dual-writes turn records into md-backed session storage, reads recap input from that store when available, threads per-session sequence numbers through segments, and archives cleaned conversations into tree leaves.
Storage, ingest, and runtime rewiring
src/openhuman/memory/ingest_pipeline.rs, src/openhuman/memory/tree_rpc.rs, src/openhuman/memory_tree/global/*, src/openhuman/memory_tree/sources/*, src/core/*, src/bin/*, src/openhuman/composio/providers/*, src/openhuman/memory_store/content/*
Runtime entrypoints, binaries, provider ingest paths, tree RPCs, tree source/global flows, and content readers now target memory, memory_store, and memory_sync module locations.
Retrieval and scoring migration
src/openhuman/memory/read_rpc.rs, src/openhuman/memory/retrieval/*, src/openhuman/memory/score/*, src/openhuman/memory/stm_recall/*, src/openhuman/memory_graph/*, src/openhuman/memory_store/retrieval/*
Retrieval, read RPCs, scoring, recall, graph derivation, and related tests are moved onto the new chunk/tree/chat interfaces and expanded with more edge-case coverage.
Queue and background job migration
src/openhuman/memory_queue/*, src/openhuman/memory_queue/handlers/mod.rs
Background job handlers, scheduler, worker, store, and backfill state now use the memory_queue and memory::jobs paths, updated logging, and new tests for dedupe, defer, and idle behavior.
Tools, entities, and store-facing features
src/openhuman/memory_store/tools/*, src/openhuman/memory_entities/*, src/openhuman/memory_tools/*, src/openhuman/memory_store/contacts/mod.rs, src/openhuman/memory_store/kv.rs
Adds raw memory_store tools, markdown-backed entity storage, tool-scoped memory tools and mock backend, contact facade helpers, and expanded KV/graph/document tests.
Docs, schemas, and migration cleanup
src/openhuman/memory/README.md, src/openhuman/memory_store/README.md, src/openhuman/memory_tree/README.md, src/openhuman/memory_sync/README.md, src/openhuman/memory/schemas/*, src/openhuman/memory/store/agentmemory/*, src/openhuman/memory_tree/chat/*
Architecture docs are rewritten for the new layering, controller schema tests are added, and the legacy agentmemory backend plus legacy memory_tree chat modules are removed.

Smoke test mock-server lifecycle

Layer / File(s) Summary
Mock server setup and teardown
app/test/e2e/specs/smoke.spec.ts
The smoke suite starts the mock server before waiting for and resetting the app, then stops the mock server after the suite completes.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Suggested labels

memory, agent

Suggested reviewers

  • graycyrus
  • M3gA-Mind

🐇 I packed old paths in a tidy sack,
And hopped new modules onto the track.
With trees and turns in markdown burrows,
The queues now drum their midnight furrows.
A mock server bows, then waves goodbye—
While little tests go bounding by.

@coderabbitai coderabbitai Bot added agent Built-in agents, prompts, orchestration, and agent runtime in src/openhuman/agent/. memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/. labels May 24, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/openhuman/memory_store/unified/graph.rs (1)

15-101: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add diagnostic logging to the new graph read/write flows.

These new graph paths mutate and query critical state but currently have no entry/branch/exit diagnostics. Please add stable-prefix debug logs (namespace/document/relation identifiers + updated/deleted counts + completion) so production triage can reconstruct behavior.

As per coding guidelines: Debug logging must follow these rules: default to verbose diagnostics on new/changed flows; log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors... All changes lacking diagnosis logging are incomplete.

Also applies to: 183-320, 322-393

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/unified/graph.rs` around lines 15 - 101, Add
verbose, stable-prefix debug logging to the new graph read/write flows (at
minimum inside graph_remove_document_namespace and the related
graph_relations_namespace calls): log an entry message with namespace and
document_id, log before/after the graph_relations_namespace call, log
per-relation branch decisions (touches_document true/false) including relation
identifiers (subject/predicate/object), log whether you performed a DELETE vs
UPDATE and counts of document_ids/chunk_ids removed/remaining, log the updated
attrs/evidence_count and updated_at values, and log commit success or error
paths (include the error text) with the same stable prefix so production triage
can reconstruct behavior. Ensure logs use the existing processLogger/debug
logger used in this module and include the unique symbols relation.subject,
relation.predicate, relation.object, document_id, updated_at, and the tx
commit/execute error contexts.
src/openhuman/memory_store/chunks/store.rs (1)

1353-1363: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Set the backfill-in-progress flag only after commit succeeds.

On Line 1359, set_backfill_in_progress(true) is called before tx.commit(). If commit fails, process-global state says backfill is active even though no durable enqueue happened.

Proposed fix
-    if has_uncovered {
+    let should_mark_backfill_in_progress = has_uncovered;
+    if has_uncovered {
         let backfill_job = crate::openhuman::memory::jobs::types::NewJob::reembed_backfill(
             &crate::openhuman::memory::jobs::types::ReembedBackfillPayload {
                 signature: sig.clone(),
             },
         )?;
         crate::openhuman::memory::jobs::enqueue_tx(&tx, &backfill_job)?;
-        crate::openhuman::memory::jobs::set_backfill_in_progress(true);
     }

     tx.commit()?;
+    if should_mark_backfill_in_progress {
+        crate::openhuman::memory::jobs::set_backfill_in_progress(true);
+    }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/chunks/store.rs` around lines 1353 - 1363, The
global backfill flag is currently set before the DB transaction is committed;
move the call to crate::openhuman::memory::jobs::set_backfill_in_progress(true)
so it only runs after the transaction and subsequent pragma update succeed
(i.e., after tx.commit()? and conn.pragma_update(...) return Ok). Specifically,
keep creating the NewJob via NewJob::reembed_backfill and enqueue_tx(&tx,
&backfill_job) inside the transaction, but remove the pre-commit
set_backfill_in_progress call and invoke set_backfill_in_progress(true) only
after tx.commit() and conn.pragma_update(...) complete successfully; propagate
errors as before if commit/pragma_update fail.
🟠 Major comments (26)
src/openhuman/memory_tree/sources/registry.rs-23-27 (1)

23-27: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Redact scope before logging to prevent PII leakage.

Line 23 and Line 26 log raw scope, which can include identifiable values (for example account email–derived scopes). Log only a redacted/hash form.

🔧 Proposed fix
 use anyhow::Result;
 
 use crate::openhuman::config::Config;
+use crate::openhuman::memory::util::redact::redact;
 use crate::openhuman::memory_tree::sources::file;
 use crate::openhuman::memory_tree::tree::registry::get_or_create_tree;
 use crate::openhuman::memory_store::trees::types::{Tree, TreeKind};
@@
 pub fn get_or_create_source_tree(config: &Config, scope: &str) -> Result<Tree> {
-    log::debug!("[sources::registry] get_or_create_source_tree scope={scope}");
+    let scope_hash = redact(scope);
+    log::debug!("[sources::registry] get_or_create_source_tree scope_hash={scope_hash}");
@@
     if let Err(e) = file::write_source_file(config, &tree) {
-        log::warn!("[sources::registry] write_source_file failed scope={scope} err={e:#}");
+        log::warn!(
+            "[sources::registry] write_source_file failed scope_hash={scope_hash} err={e:#}"
+        );
     }
     Ok(tree)
 }

As per coding guidelines, "never log secrets or full PII."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_tree/sources/registry.rs` around lines 23 - 27, The logs
in src/openhuman/memory_tree/sources/registry.rs print the raw scope (in the
debug at get_or_create_source_tree and the warn when file::write_source_file
fails), which may contain PII; replace direct use of scope in log messages with
a redacted or hashed representation (e.g., compute a short SHA256/hex or mask
function) and log that instead, keeping the descriptive context (e.g.,
"scope_redacted" or "scope_hash") in the log calls where log::debug! and
log::warn! are used so get_or_create_tree, file::write_source_file error paths
and their messages no longer emit raw scope values.
src/openhuman/memory_store/tools/kinds.rs-28-32 (1)

28-32: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add diagnostic logging to the tool execution path.

This new runtime flow currently has no diagnostics. Please add entry/exit and success/failure logs with a stable grep-friendly prefix and correlation fields (tool/method + count).

🔧 Proposed patch
 async fn execute(&self, _args: Value) -> anyhow::Result<ToolResult> {
+    tracing::debug!(
+        "[domain][memory_store_kinds] execute start method=memory_store_kinds"
+    );
     let kinds: Vec<&'static str> = MemoryKind::ALL.iter().map(|k| k.as_str()).collect();
     let json = serde_json::to_string(&json!({ "kinds": kinds }))?;
+    tracing::debug!(
+        "[domain][memory_store_kinds] execute success method=memory_store_kinds kinds_count={}",
+        MemoryKind::ALL.len()
+    );
     Ok(ToolResult::success(json))
 }

As per coding guidelines: "Debug logging must follow these rules... All changes lacking diagnosis logging are incomplete."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/tools/kinds.rs` around lines 28 - 32, Add
diagnostic logging to the execute method: log an "ENTRY" with a stable
grep-friendly prefix (e.g., "DIAG:memory_kinds:enter") including fields
tool="memory_kinds" method="execute" and a correlation id or counter, log
success ("DIAG:memory_kinds:success") with the produced kinds count and the same
correlation fields before returning ToolResult::success, and catch/convert
errors to log a failure ("DIAG:memory_kinds:failure") with the error details and
correlation fields; reference the execute function, MemoryKind::ALL and
ToolResult::success when adding these logs.
src/openhuman/memory/README.md-40-54 (1)

40-54: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix broken local links in the module map table.

The links added in this section currently fail Markdown Link Check (missing targets for jobs, canonicalize, chat, conversations, summarizer, sync_status, tree_global, and tree_topic). Please update these references to valid existing paths so docs CI passes.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory/README.md` around lines 40 - 54, Update the broken
Markdown links in the module map table so they point to actual existing
directories/files: find the table entries for `jobs/`, `canonicalize/`, `chat/`,
`conversations/`, `summarizer/`, `sync_status/`, `tree_global/`, and
`tree_topic/` in README.md and change their link targets to the correct relative
paths that exist in the repo (e.g., add missing trailing filenames or correct
directory names to match the filesystem), ensuring each bracketed name maps to a
valid target so markdown link check passes; verify by opening each target
locally or running the docs CI check.
src/core/jsonrpc.rs-1561-1561 (1)

1561-1561: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Move memory job bootstrap out of src/core/jsonrpc.rs.

This direct domain startup call keeps domain-specific behavior in the transport layer. Please shift memory job bootstrap behind domain/controller startup wiring and keep src/core orchestration transport-focused.

As per coding guidelines: src/core/ modules are transport-only and must not contain heavy domain logic. Domain branches must be removed from src/core/cli.rs and src/core/jsonrpc.rs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/jsonrpc.rs` at line 1561, Remove the direct domain startup call
crate::openhuman::memory::jobs::start from the transport layer (the jsonrpc
bootstrap) and instead invoke it from the domain/controller startup wiring where
domain services are initialized; specifically delete the call in jsonrpc.rs (and
any similar call in cli.rs) and add the startup invocation to the domain
controller initialization routine (the module that wires up openhuman
services/controllers) so memory job bootstrapping runs as part of domain/service
startup rather than in the transport code.
src/openhuman/memory_graph/mod.rs-28-32 (1)

28-32: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add schemas wiring for this new domain module.

This src/openhuman/*/mod.rs currently exposes query/types only. Please add mod schemas; plus all_memory_graph_controller_schemas and all_memory_graph_registered_controllers re-exports so it follows the controller-registry contract.

Suggested patch
 pub mod query;
+mod schemas;
 pub mod types;
 
 pub use query::{co_occurring_entities, neighbors};
+pub use schemas::{
+    all_controller_schemas as all_memory_graph_controller_schemas,
+    all_registered_controllers as all_memory_graph_registered_controllers,
+};
 pub use types::GraphEdge;

As per coding guidelines: src/openhuman/*/mod.rs: add mod schemas; and re-export all_controller_schemas as all_<domain>_controller_schemas plus all_registered_controllers as all_<domain>_registered_controllers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_graph/mod.rs` around lines 28 - 32, Add a new schemas
module and re-export the controller registry symbols: add `mod schemas;` to this
module and re-export `schemas::all_controller_schemas` renamed to
`all_memory_graph_controller_schemas` and `schemas::all_registered_controllers`
renamed to `all_memory_graph_registered_controllers` so the file exposes the
controller-schema and registered-controller hooks; locate this change near the
existing `pub use query::{co_occurring_entities, neighbors};` and `pub use
types::GraphEdge;` declarations and add the two re-exports with the specified
names.
src/openhuman/memory_store/unified/init.rs-151-156 (1)

151-156: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not treat every migration error as “already exists.”

The new segments migration loop currently downgrades all SQL errors to trace and continues. That can silently skip real failures (bad SQL, DB corruption, permission issues) while startup appears successful. Please only ignore the specific duplicate-column case and return errors for anything else.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/unified/init.rs` around lines 151 - 156, The loop
that applies super::segments::SEGMENTS_MIGRATIONS_SQL currently treats every
error as benign; change the match on conn.execute(sql, []) so you only ignore
the specific "duplicate column" / "already exists" SQLite error and propagate
all other errors. Concretely, match rusqlite::Error (e.g.,
rusqlite::Error::SqliteFailure(code, msg)) and if the message/text contains
"duplicate column" or "already exists" log a trace and continue, otherwise
return or propagate the error (do not swallow it); keep references to
SEGMENTS_MIGRATIONS_SQL and conn.execute(sql, []) so the fix is applied in the
same loop.
src/openhuman/memory_store/tools/raw_chunks.rs-88-95 (1)

88-95: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce limit bounds in runtime, not only JSON schema.

execute() trusts parsed.limit and forwards it directly. Callers that bypass schema validation can pass very large values and trigger expensive scans. Add an explicit guard in code (e.g., reject values outside 1..=1000) before building ListChunksQuery.

Suggested fix
         let source_kind = match parsed.source_kind.as_deref() {
             Some(s) => Some(
                 SourceKind::parse(s)
                     .map_err(|e| anyhow::anyhow!("memory_store_raw_chunks: {e}"))?,
             ),
             None => None,
         };
+        if let Some(limit) = parsed.limit {
+            if !(1..=1000).contains(&limit) {
+                return Err(anyhow::anyhow!(
+                    "memory_store_raw_chunks: limit must be between 1 and 1000"
+                ));
+            }
+        }
         let query = ListChunksQuery {
             source_kind,
             source_id: parsed.source_id,
             owner: parsed.owner,
             since_ms: parsed.since_ms,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/tools/raw_chunks.rs` around lines 88 - 95, The
code currently trusts parsed.limit and forwards it into ListChunksQuery inside
execute(), allowing callers to bypass schema checks and trigger huge scans;
update the execute() function to validate parsed.limit at runtime (e.g., ensure
parsed.limit is within 1..=1000) and return an error (or clamp to a safe
default) if it is out of range before constructing ListChunksQuery, referencing
parsed.limit, execute(), and ListChunksQuery to locate where to add the guard.
src/openhuman/memory_store/contacts/mod.rs-22-47 (1)

22-47: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add diagnostic logging to fail-soft branches.

All three helpers drop initialization/read errors without emitting any trace/debug/warn context, which makes contact-resolution failures opaque in production diagnostics.

As per coding guidelines, "Debug logging must follow these rules: default to verbose diagnostics on new/changed flows ... and errors; ... All changes lacking diagnosis logging are incomplete."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/contacts/mod.rs` around lines 22 - 47, Each helper
(get_contact, list_contacts, lookup_contact) must emit diagnostic logs on
fail-soft paths: when get_store() returns Err and when the awaited store calls
(s.get, s.list, s.lookup) return Err/None. Update the Err(_) arms to log the
initialization error (use tracing::warn or tracing::debug) including context
like the function name and person_id/handle where applicable, and log when
s.get(...).await.ok() / s.lookup(...).await.ok() are None or
s.list().await.unwrap_or_default() fell back, so callers and ops are visible in
diagnostics; reference get_contact, list_contacts, lookup_contact and
get_store/s.get/s.list/s.lookup to locate the spots to add the logs.
src/openhuman/memory_archivist/store.rs-31-35 (1)

31-35: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid lossy session-id normalization to prevent cross-session collisions.

Different session IDs can collapse to the same directory name, which can mix archived turns across sessions.

💡 Proposed fix
 fn sanitize_session(s: &str) -> String {
-    s.chars()
-        .map(|c| if c.is_alphanumeric() || c == '-' || c == '_' { c } else { '_' })
-        .collect()
+    let mut out = String::with_capacity(s.len());
+    for b in s.bytes() {
+        match b {
+            b'0'..=b'9' | b'a'..=b'z' | b'A'..=b'Z' | b'-' => out.push(char::from(b)),
+            b'_' => out.push_str("__"), // escape literal underscore
+            _ => out.push_str(&format!("_{b:02x}")),
+        }
+    }
+    out
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_archivist/store.rs` around lines 31 - 35, The current
sanitize_session function maps all non-alphanumeric/'-'/'_' chars to '_' which
can collapse distinct session IDs; replace this lossy normalization with a
reversible, collision-resistant encoding such as percent-encoding (URL-encode)
or a URL-safe base64/SHA256 fingerprint to preserve uniqueness. Update
sanitize_session to either (a) percent-encode the input session string
preserving allowed chars and encoding others, or (b) return a deterministic
short hash (e.g., sha256 hex or url-safe base64) of the full session string when
used as a filesystem directory name; ensure the function name sanitize_session
remains and that callers using its return value continue to get a unique,
filesystem-safe string.
src/openhuman/memory_archivist/store.rs-103-109 (1)

103-109: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not log raw session_id values.

This can leak user identifiers in debug logs; redact or hash before logging.

💡 Proposed fix
 use crate::openhuman::config::Config;
 use crate::openhuman::memory_archivist::types::ArchivedTurn;
+use crate::openhuman::memory::util::redact::redact;
 use crate::openhuman::memory_store::content::atomic::write_if_new;
@@
     log::debug!(
         "[memory_archivist] recorded session={} seq={} role={} bytes={}",
-        turn.session_id,
+        redact(&turn.session_id),
         turn.seq,
         turn.role,
         bytes.len()
     );
As per coding guidelines: "never log secrets or full PII."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_archivist/store.rs` around lines 103 - 109, Do not log
raw session IDs: replace direct use of turn.session_id in the debug call with a
non-reversible redaction or hash (e.g., compute a SHA-256 or HMAC of
turn.session_id or redact to a short token) and log that instead; update the
log::debug call that currently references turn.session_id (alongside turn.seq,
turn.role, bytes.len()) to use the hashed/redacted value so full PII is never
written to logs.
src/openhuman/memory_archivist/store.rs-94-103 (1)

94-103: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

record_turn has a sequence assignment race under concurrent writes.

next_seq() and write_if_new() are separated by a TOCTOU window. Two concurrent writers can pick the same seq and one append fails, dropping a turn.

💡 Suggested direction
 pub fn record_turn(config: &Config, mut turn: ArchivedTurn) -> Result<ArchivedTurn> {
     let dir = session_dir(config, &turn.session_id);
     fs::create_dir_all(&dir)
         .with_context(|| format!("failed to mkdir -p {}", dir.display()))?;
-    turn.seq = next_seq(&dir);
-    let path = dir.join(format!("{:06}.md", turn.seq));
-    let bytes = compose_turn(&turn).into_bytes();
-    write_if_new(&path, &bytes)
-        .with_context(|| format!("failed to write episodic turn {}", path.display()))?;
+    let bytes_base = compose_turn(&turn);
+    for _ in 0..16 {
+        turn.seq = next_seq(&dir);
+        let path = dir.join(format!("{:06}.md", turn.seq));
+        let mut bytes = bytes_base.clone().into_bytes();
+        // recompute content with assigned seq if seq is encoded in frontmatter
+        bytes = compose_turn(&turn).into_bytes();
+        match write_if_new(&path, &bytes) {
+            Ok(_) => return Ok(turn),
+            Err(err) if path.exists() => continue, // collision, retry with next seq
+            Err(err) => {
+                return Err(err)
+                    .with_context(|| format!("failed to write episodic turn {}", path.display()));
+            }
+        }
+    }
+    anyhow::bail!("failed to reserve unique seq for session {}", turn.session_id);
-    log::debug!( ... );
-    Ok(turn)
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_archivist/store.rs` around lines 94 - 103, record_turn
has a TOCTOU race between next_seq() and write_if_new() causing duplicate seq
under concurrency; fix by making seq assignment and file creation atomic: in
record_turn, loop getting seq via next_seq(dir) then attempt to create the
target file using an exclusive create (create_new / O_EXCL) rather than writing
conditionally, and only on successful open/creation write the bytes and set
turn.seq; if create fails because file exists, increment seq (or retry next_seq)
and repeat until a successful exclusive create; reference functions record_turn,
next_seq, and write_if_new to locate where to replace the current two-step logic
with the atomic create-and-write retry loop.
src/openhuman/memory_entities/mod.rs-58-62 (1)

58-62: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add domain schema exports to memory_entities module.

memory_entities/mod.rs should include mod schemas; and re-export the domain controller schema/registry aliases so this domain follows the controller migration contract.

Suggested patch
 pub mod store;
+pub mod schemas;
 pub mod types;
 
+pub use schemas::{
+    all_controller_schemas as all_memory_entities_controller_schemas,
+    all_registered_controllers as all_memory_entities_registered_controllers,
+};
 pub use store::{get_entity, list_entities, lookup_alias, put_entity};
 pub use types::{Entity, EntityHandle, EntityKind};

As per coding guidelines, "src/openhuman/*/mod.rs: Add mod schemas; in the domain mod.rs and re-export all_controller_schemas and all_registered_controllers."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_entities/mod.rs` around lines 58 - 62, Add the domain
schema module and re-exports to the memory_entities module: declare the schemas
submodule by adding mod schemas; in src/openhuman/memory_entities/mod.rs and
re-export the controller schema aliases so the domain meets the controller
migration contract (add a pub use of schemas::{all_controller_schemas,
all_registered_controllers}). Ensure you reference the existing module file
(schemas) and the exported symbols all_controller_schemas and
all_registered_controllers from that module.
src/openhuman/memory_graph/query.rs-16-77 (1)

16-77: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add required diagnostics for new graph-query flows.

These new runtime query paths should emit entry/exit diagnostics (including cap and result count) to satisfy the Rust diagnostics policy.

Suggested patch
 pub fn co_occurring_entities(
     config: &Config,
     subject_entity: &str,
     limit: Option<usize>,
 ) -> Result<Vec<GraphEdge>> {
     let cap = limit.unwrap_or(100).min(i64::MAX as usize) as i64;
+    log::debug!(
+        "[domain][memory_graph::query] co_occurring_entities start subject_chars={} cap={}",
+        subject_entity.chars().count(),
+        cap
+    );
     with_connection(config, |conn| {
@@
-        Ok(rows
+        let edges = rows
             .into_iter()
             .map(|(object, weight)| GraphEdge {
                 subject: subject_entity.to_string(),
                 object,
                 weight: weight.max(0) as u32,
             })
-            .collect())
+            .collect::<Vec<_>>();
+        log::debug!(
+            "[domain][memory_graph::query] co_occurring_entities done subject_chars={} rows={}",
+            subject_entity.chars().count(),
+            edges.len()
+        );
+        Ok(edges)
     })
 }

As per coding guidelines, "Debug logging must follow these rules: default to verbose diagnostics on new/changed flows... All changes lacking diagnosis logging are incomplete."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_graph/query.rs` around lines 16 - 77, The new
graph-query flows (co_occurring_entities, neighbors, group_by_weight) lack
entry/exit diagnostics; add structured debug logs at the start and end of
co_occurring_entities (and propagate/log in neighbors) that record the
subject_entity, computed cap (limit), query success/failure, and the resulting
row/edge count; use the existing with_connection closure around the SQL to emit
an entry log before preparing the statement and an exit log after mapping into
Vec<GraphEdge> (include weight counts), and ensure errors include context when
logging failures so diagnostics show cap and result_count for observability.
src/openhuman/memory_tools/tools/list.rs-44-57 (1)

44-57: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add full diagnostic logging for the new tool execution path.

Line 47 logs entry, but this new flow is missing exit/error/external-call diagnostics (client acquisition, store query result count, failure branches). Please add structured debug/warn logs before each early error return and on success completion.

Suggested patch
 async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
+    let started = std::time::Instant::now();
     let parsed: Args = serde_json::from_value(args)
-        .map_err(|e| anyhow::anyhow!("invalid arguments for memory_tools_list: {e}"))?;
+        .map_err(|e| {
+            log::warn!("[tool][memory_tools] list invalid_args error={e}");
+            anyhow::anyhow!("invalid arguments for memory_tools_list: {e}")
+        })?;
     log::debug!("[tool][memory_tools] list tool_name={}", parsed.tool_name);
     let client = active_memory_client()
         .await
-        .map_err(|e| anyhow::anyhow!("memory_tools_list: {e}"))?;
+        .map_err(|e| {
+            log::warn!("[tool][memory_tools] list active_memory_client_failed error={e}");
+            anyhow::anyhow!("memory_tools_list: {e}")
+        })?;
     let store = ToolMemoryStore::new(client.memory_handle());
     let rules = store
         .list_rules(&parsed.tool_name)
         .await
-        .map_err(|e| anyhow::anyhow!("memory_tools_list: {e}"))?;
+        .map_err(|e| {
+            log::warn!(
+                "[tool][memory_tools] list_rules_failed tool_name={} error={e}",
+                parsed.tool_name
+            );
+            anyhow::anyhow!("memory_tools_list: {e}")
+        })?;
+    log::debug!(
+        "[tool][memory_tools] list success tool_name={} count={} elapsed_ms={}",
+        parsed.tool_name,
+        rules.len(),
+        started.elapsed().as_millis()
+    );
     let json = serde_json::to_string(&rules)?;
     Ok(ToolResult::success(json))
 }
As per coding guidelines: “Debug logging must follow these rules: default to verbose diagnostics on new/changed flows; log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_tools/tools/list.rs` around lines 44 - 57, The execute
method for the list tool needs additional structured debug/warn logs:
before/after calling active_memory_client() log an entry like "acquiring memory
client" and include any error info on failure (refer to active_memory_client()),
before/after calling ToolMemoryStore::list_rules(&parsed.tool_name) log the
external call and on success log the returned rules count and tool_name (refer
to ToolMemoryStore and list_rules), add debug logs on the early map_err failure
branches that include the error details (the same places returning
anyhow::anyhow!("memory_tools_list: {e}") and the serde_json::from_value error
for parsed), and log an explicit exit/success message just before returning
ToolResult::success(json) (refer to ToolResult::success and parsed.tool_name);
keep messages structured and include relevant variables (tool_name, error,
rules.len()).
src/openhuman/memory_archivist/clip.rs-10-21 (1)

10-21: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add lightweight diagnostics for conversation clipping decisions.

This new transform path currently has no diagnostics. Add a trace/debug log with input turn count and dropped tool-turn count so clipping behavior is inspectable.

As per coding guidelines: “Debug logging must follow these rules… log entry/exit, branches… and errors… All changes lacking diagnosis logging are incomplete.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_archivist/clip.rs` around lines 10 - 21,
clean_conversation currently strips "tool" turns without emitting any
diagnostics; add a lightweight trace-level log at function entry/exit that
records the input turns length and the number of dropped tool turns so clipping
decisions are inspectable. Specifically, in the clean_conversation function
(working with Turn), compute input_count = turns.len() and dropped_count =
turns.iter().filter(|t| t.role == "tool").count(), emit a trace/debug log (using
the project's logging macro, e.g. tracing::trace! or log::debug!) with those two
values before/after the transformation, and include a short message indicating
the function name and that tool turns were dropped. Ensure logging is
non-allocating (use counts, not full turn payloads) and does not change return
behavior.
src/openhuman/memory_archivist/tree_writer.rs-37-84 (1)

37-84: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add branch/error diagnostics around the archivist tree write path.

This new flow logs only the success tail. Please add entry + branch/error logs around the append_leaf call so failures and write-path decisions are traceable in production.

Proposed logging-focused patch
 pub async fn archive_to_tree(
     config: &Config,
     tree: &Tree,
     session_id: &str,
     turns: &[Turn],
 ) -> Result<TreeWriteOutcome> {
+    log::debug!(
+        "[memory_archivist] archive_to_tree start tree_id={} session={} turns={}",
+        tree.id,
+        session_id,
+        turns.len()
+    );
     let cleaned = clean_conversation(turns);
     let md = compose_conversation_md(&cleaned);
@@
-    let new_summary_ids = append_leaf(config, tree, &leaf_ref, &strategy).await?;
+    let new_summary_ids = match append_leaf(config, tree, &leaf_ref, &strategy).await {
+        Ok(ids) => ids,
+        Err(err) => {
+            log::warn!(
+                "[memory_archivist] archive_to_tree append_leaf failed tree_id={} session={} chunk_id={} err={:#}",
+                tree.id,
+                session_id,
+                chunk_id,
+                err
+            );
+            return Err(err);
+        }
+    };
     log::debug!(
         "[memory_archivist] archive_to_tree tree_id={} session={} chunk_id={} new_summaries={}",
         tree.id,

As per coding guidelines: “Debug logging must follow these rules… log entry/exit, branches, external calls… and errors… All changes lacking diagnosis logging are incomplete.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_archivist/tree_writer.rs` around lines 37 - 84, Add
diagnostic logging around the archivist tree write path in archive_to_tree: log
entry with session_id/tree.id/chunk_id and label_strategy (e.g., when building
TreeWriteRequest and before calling append_leaf), log the decision branch (using
strategy = LabelStrategy::Empty) and the external call to append_leaf, and
capture+log any error returned from append_leaf with context (session_id,
tree_id, chunk_id, and the TreeWriteRequest/leaf summary identifiers) before
returning the Err; keep the existing success debug log and include the appended
new_summary_ids there. Ensure logs use log::debug for normal branches and
log::error for failures and reference archive_to_tree, append_leaf,
TreeWriteRequest, and LabelStrategy::Empty so future readers can locate the
changes.
src/openhuman/memory_tools/store.rs-109-169 (1)

109-169: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Log which list_rules backend path is used and the loaded count.

The new sqlite-vs-memory branch has no explicit path/result diagnostics. Please log selected backend and resulting rule count for this changed flow.

Suggested minimal instrumentation
     pub async fn list_rules(&self, tool_name: &str) -> Result<Vec<ToolMemoryRule>, String> {
         let namespace = tool_memory_namespace(tool_name);
         let mut rules: Vec<ToolMemoryRule> = if let Some(conn) = self.memory.sqlite_conn() {
+            log::debug!(
+                "[tool-memory] list_rules using sqlite backend tool={} namespace={}",
+                tool_name, namespace
+            );
             let conn = conn.lock();
@@
-            rows.filter_map(|row| match row {
+            let out: Vec<ToolMemoryRule> = rows.filter_map(|row| match row {
                 Ok((key, content)) => match serde_json::from_str::<ToolMemoryRule>(&content) {
@@
             })
-            .collect()
+            .collect();
+            log::debug!("[tool-memory] list_rules sqlite loaded={} tool={}", out.len(), tool_name);
+            out
         } else {
+            log::debug!(
+                "[tool-memory] list_rules using memory.list backend tool={} namespace={}",
+                tool_name, namespace
+            );
             let entries = self
@@
-            entries
+            let out: Vec<ToolMemoryRule> = entries
                 .into_iter()
@@
-                .collect()
+                .collect();
+            log::debug!("[tool-memory] list_rules memory.list loaded={} tool={}", out.len(), tool_name);
+            out
         };

As per coding guidelines: “Debug logging must follow these rules… log entry/exit, branches, external calls… and errors… All changes lacking diagnosis logging are incomplete.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_tools/store.rs` around lines 109 - 169, In list_rules,
add debug logs that indicate which backend path was taken (sqlite vs in-memory)
and the number of rules loaded for the given tool; specifically, inside the
branch that checks self.memory.sqlite_conn() log that the "sqlite" backend was
selected before/after collecting rules (reference function list_rules and call
to self.memory.sqlite_conn()), and inside the else branch that calls
self.memory.list(...) log that the "memory" backend was selected; after the
rules Vec is produced (variable rules) emit a log entry with tool_name and
rules.len() so callers can see backend and loaded count.
src/openhuman/memory_store/traits.rs-79-87 (1)

79-87: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Front-matter fields are emitted unescaped, which can generate invalid YAML metadata.

Raw interpolation into YAML keys/values risks malformed files for legitimate inputs (e.g., names/source IDs containing YAML-significant characters/newlines). Please serialize scalar values safely before embedding them in front-matter across all to_obsidian implementations.

Proposed direction
+fn yaml_scalar(value: &str) -> String {
+    // JSON string encoding is valid YAML scalar syntax.
+    serde_json::to_string(value).unwrap_or_else(|_| "\"\"".to_string())
+}

Then use yaml_scalar(...) for front-matter values (id/scope/source_id/display_name/email/etc.) instead of raw {} interpolation.

Also applies to: 109-112, 123-125, 159-162

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/traits.rs` around lines 79 - 87, The front-matter
is being composed by raw formatting (in to_obsidian implementations) which can
emit unescaped YAML and break files; modify the to_obsidian code in traits.rs to
serialize every front-matter scalar using the existing yaml_scalar(...) helper
instead of raw interpolation for fields like self.id,
self.metadata.source_kind.as_str(), self.metadata.source_id, self.seq_in_source
(and other metadata fields such as display_name/email where present), and keep
tags_yaml/content separate; ensure tags_yaml remains properly serialized and
replace each raw {} interpolation in those front-matter blocks (including the
other ranges called out) with the yaml_scalar(...) wrapped values so all emitted
YAML scalars are safe.
src/openhuman/memory_store/retrieval/mod.rs-72-147 (1)

72-147: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add debug/trace diagnostics to the new retrieval facade methods.

tree_walk, vector_search, keyword_search, and param_tag_search are newly introduced flow entrypoints but currently have no entry/exit/branch diagnostics. Please add structured debug logs (without query/body PII) including method name, namespace/tree/source filters, limits, and result counts.

As per coding guidelines: “Debug logging must follow these rules: default to verbose diagnostics on new/changed flows … All changes lacking diagnosis logging are incomplete.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/retrieval/mod.rs` around lines 72 - 147, Add
structured debug/trace logging at entry, key branches, and exit for tree_walk,
vector_search, keyword_search, and param_tag_search: at method entry log method
name and non-PII identifiers (e.g., node_id, namespace, presence/length or hash
of query rather than the raw query, limit/max_depth, and a compact summary of
ParamTagFilters like source_kind/source_id/owner/since_ms/until_ms/limit); after
calling underlying functions log result counts and any Err (include error
details); inside param_tag_search log the branch where tags_all_of is None/empty
and the final filtered count; use the existing tracing/log facility (e.g.,
tracing::debug!) and ensure no raw query/body PII is emitted.
src/openhuman/memory/ops/documents.rs-492-709 (1)

492-709: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Move this inline test module into a sibling documents_test.rs.

This addition pushes the implementation file past the ~500-line threshold and bundles a large test suite directly into the operational module. Please wire it as:

#[cfg(test)] #[path = "documents_test.rs"] mod tests;

As per coding guidelines: src/**/*.rs: “When extracting Rust tests out of an implementation file, prefer a sibling *_test.rs file wired in with #[cfg(test)] #[path = "..._test.rs"] mod tests;” and **/*.{ts,tsx,rs}: “File size should not exceed approximately 500 lines.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory/ops/documents.rs` around lines 492 - 709, The inline
#[cfg(test)] mod tests block in documents.rs is too large and should be moved to
a sibling file; extract the entire tests module into a new sibling file named
documents_test.rs and in documents.rs replace the inline module with a one-line
module declaration: #[cfg(test)] #[path = "documents_test.rs"] mod tests; ensure
the new file contains the exact test code (including helper functions like
ensure_memory_client, unique_namespace, sample_put and the async #[tokio::test]
functions) and that any module-private items it depends on remain accessible or
are made pub(crate) as needed.
src/openhuman/memory_store/retrieval/mod.rs-124-147 (1)

124-147: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Apply limit after tags_all_of filtering to avoid dropping valid matches.

ListChunksQuery.limit is applied before tags_all_of post-filtering, so matching rows beyond the initial limited window are never considered. This can produce false negatives for tag-constrained queries.

Proposed fix
-        let query = crate::openhuman::memory_store::chunks::store::ListChunksQuery {
+        let post_filter_tags = filters
+            .tags_all_of
+            .as_ref()
+            .is_some_and(|required| !required.is_empty());
+        let query = crate::openhuman::memory_store::chunks::store::ListChunksQuery {
             source_kind: filters.source_kind,
             source_id: filters.source_id.clone(),
             owner: filters.owner.clone(),
             since_ms: filters.since_ms,
             until_ms: filters.until_ms,
-            limit: filters.limit,
+            // Apply row cap after tag post-filtering to preserve correctness.
+            limit: if post_filter_tags { None } else { filters.limit },
         };
-        let rows = list_chunks(config, &query)?;
+        let mut rows = list_chunks(config, &query)?;
         let Some(required) = filters.tags_all_of.as_ref() else {
             return Ok(rows);
         };
         if required.is_empty() {
             return Ok(rows);
         }
-        Ok(rows
-            .into_iter()
+        rows = rows.into_iter()
             .filter(|c| {
                 required
                     .iter()
                     .all(|t| c.metadata.tags.iter().any(|ct| ct == t))
             })
-            .collect())
+            .collect();
+        if let Some(limit) = filters.limit {
+            rows.truncate(limit);
+        }
+        Ok(rows)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/retrieval/mod.rs` around lines 124 - 147, The
current flow constructs ListChunksQuery
(source_kind/source_id/owner/since_ms/until_ms/limit) and calls list_chunks
which applies the limit before we post-filter by filters.tags_all_of, causing
valid matches beyond the DB limit to be lost; change the logic in retrieval::...
(the function that builds query and calls list_chunks) to not set query.limit
(or set it to None/large) when filters.tags_all_of is present, fetch an
un-limited or sufficiently large rows set via list_chunks, then apply the
tags_all_of filtering on rows (the existing filter using
required.iter().all(...)) and only apply the final limit after filtering;
reference symbols: ListChunksQuery, list_chunks, filters.tags_all_of, rows, and
the post-filter closure.
src/openhuman/memory_tree/summarise.rs-125-131 (1)

125-131: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return an error for empty provider output to preserve the function contract.

At Line 130, raw.trim() can be empty and still returns Ok(SummaryOutput { content: "" ... }). This bypasses caller fallback paths that rely on Err for upstream empty responses.

Proposed fix
     let raw = provider
         .chat_for_text(&prompt)
         .await
         .with_context(|| format!("memory_tree::summarise: provider={}", provider.name()))?;
-
-    let (content, token_count) = clamp_to_budget(raw.trim(), effective_budget);
+    let trimmed = raw.trim();
+    if trimmed.is_empty() {
+        anyhow::bail!(
+            "memory_tree::summarise: empty response from provider={}",
+            provider.name()
+        );
+    }
+    let (content, token_count) = clamp_to_budget(trimmed, effective_budget);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_tree/summarise.rs` around lines 125 - 131, After calling
provider.chat_for_text in summarise, check the provider response after trimming
(the result of raw.trim()) and return an Err when that trimmed string is empty
so callers receive an error rather than an empty SummaryOutput; specifically, in
the summarise flow after the provider.chat_for_text call (and before
clamp_to_budget), detect if raw.trim().is_empty() and return an
anyhow/contextual error (including provider.name()) instead of continuing to
clamp_to_budget.
src/openhuman/memory/chat.rs-71-75 (1)

71-75: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add explicit error diagnostics for failed provider calls.

chat_with_history(...) failures are returned but not logged with chat context (kind, provider, model), which makes production triage harder for this new flow.

Proposed fix
-        let text = self
-            .inner
-            .chat_with_history(&messages, &self.model, prompt.temperature)
-            .await?;
+        let text = self
+            .inner
+            .chat_with_history(&messages, &self.model, prompt.temperature)
+            .await
+            .inspect_err(|e| {
+                log::warn!(
+                    "[memory::chat] provider={} kind={} model={} chat_with_history failed: {e:#}",
+                    self.display,
+                    prompt.kind,
+                    self.model
+                );
+            })?;
As per coding guidelines: `Debug logging must follow these rules ... log ... errors; ... All changes lacking diagnosis logging are incomplete.`
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory/chat.rs` around lines 71 - 75, The call to
self.inner.chat_with_history(&messages, &self.model, prompt.temperature).await?
can fail but the error is returned without any diagnostic logging; wrap the
await call to capture errors, log an error message that includes the chat
context (e.g. kind, provider, model — use the struct fields or variables
available on self and/or prompt) plus the original error, and then propagate the
error (return Err). Specifically, replace the direct ? with explicit match or
map_err that logs using the project's logger (or tracing::error) and includes
self.model, provider/kind fields and the error, then rethrow the error so
behavior is unchanged but failures are diagnosable.
src/openhuman/memory_entities/store.rs-67-139 (1)

67-139: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add diagnostics for entity read/list/lookup flows.

put_entity logs, but get_entity, list_entities, and lookup_alias currently have no entry/branch/error diagnostics, making store troubleshooting much harder.

As per coding guidelines "Debug logging must follow these rules: default to verbose diagnostics on new/changed flows; log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors; ... All changes lacking diagnosis logging are incomplete."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_entities/store.rs` around lines 67 - 139, get_entity,
list_entities, and lookup_alias currently have no diagnostics; add entry/exit
and branch/error debug logs using the same logging facility used by put_entity
(e.g. tracing::debug or the project's logger) so store troubleshooting is
possible. In get_entity (function get_entity) log entry with kind/canonical_id,
log and return when path doesn't exist, and log success after parsing; in
list_entities (function list_entities) log entry with kind, log and return when
the directory is missing, log each skipped non-.md file and any parse failures,
and log the final count of entities returned; in lookup_alias (function
lookup_alias) log the normalized needle and log when a match is found (include
which field matched: alias/email/handle/display_name) and when no match is
found. Ensure error logs include the underlying error/context (reuse existing
with_context messages) and keep verbosity at debug/trace level per guidelines.
src/openhuman/memory_entities/store.rs-77-79 (1)

77-79: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t treat malformed entity files as “not found.”

On Line 79, a parse failure returns Ok(None), which is indistinguishable from a genuinely missing file and can mask on-disk corruption.

Proposed fix
 pub fn get_entity(
     config: &Config,
     kind: EntityKind,
     canonical_id: &str,
 ) -> Result<Option<Entity>> {
     let path = entity_path(config, kind, canonical_id);
     if !path.exists() {
         return Ok(None);
     }
     let text = fs::read_to_string(&path)
         .with_context(|| format!("failed to read {}", path.display()))?;
-    Ok(parse(&text))
+    let parsed = parse(&text)
+        .ok_or_else(|| anyhow::anyhow!("invalid entity file format: {}", path.display()))?;
+    Ok(Some(parsed))
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_entities/store.rs` around lines 77 - 79, The code
currently returns Ok(parse(&text)) so a parse failure (parse returning None) is
returned as Ok(None) and indistinguishable from a missing file; change this to
treat parse failures as errors by converting the None into an Err with
contextual info. After reading the file with read_to_string(&path), call
parse(&text) and use ok_or_else/map_err to return a proper error (e.g., include
path.display() and a message like "malformed entity file") instead of Ok(None),
so the caller receives an Err when parsing fails rather than a successful None.
src/openhuman/agent/harness/archivist.rs-666-698 (1)

666-698: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't prefer archivist rows unless the dual-write is known complete.

record_turn is explicitly best-effort for user and assistant rows, but this helper returns md-backed turns on any successful session_entries(...) call. A single silent dual-write miss now makes recap, event extraction, and tree ingest read an incomplete session instead of falling back to FTS5. Keep FTS5 as the source of truth during the migration, or only prefer the archivist read after a completeness check (for example row count / latest seq parity).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/agent/harness/archivist.rs` around lines 666 - 698,
read_session_entries currently returns archivist-backed turns whenever
session_entries(cfg, session_id) succeeds, which risks returning incomplete data
if dual-write to FTS5 failed; update read_session_entries to prefer
fts5::episodic_session_entries(conn, session_id) as the default and only use
crate::openhuman::memory_archivist::store::session_entries(cfg, session_id) when
a completeness check passes (e.g., compare row counts or latest sequence IDs
between archivist and FTS5, or verify a dual-write-complete flag written on
record_turn), otherwise fall back to FTS5; implement the check in
read_session_entries using the existing session_entries and
fts5::episodic_session_entries helpers and only map/return archivist turns when
parity/completeness is confirmed.
🟡 Minor comments (3)
src/bin/gmail_backfill_3d.rs-44-50 (1)

44-50: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale memory-tree wording in module docs.

The implementation now reads/verifies through memory_store, but the header docs still frame this as a “memory-tree” store flow, which is confusing for operators and future maintenance. Please align wording with the new module surface.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bin/gmail_backfill_3d.rs` around lines 44 - 50, The module header/docs
still refer to a “memory-tree” store flow but the implementation uses the
memory_store API; update the top-of-file/module documentation to reference the
new memory_store surface and its verification/reading flow (mentioning
functions/modules used: get_chunk_content_pointers, list_chunks,
list_summaries_with_content_path, verify_chunk_file, verify_summary_file, and
drain_until_idle) and remove or replace any “memory-tree” terminology so the
docs accurately describe the current memory_store-based behavior and operator
steps.
src/openhuman/memory_store/contacts/mod.rs-54-64 (1)

54-64: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Test is prone to global-state flakiness.

This test assumes PeopleStore is uninitialized, but singleton init in other tests can make assertions nondeterministic in shared test processes. Please isolate the precondition (or assert fail-soft behavior without requiring uninitialized global state).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory_store/contacts/mod.rs` around lines 54 - 64, The test
contact_facade_fail_softs_when_store_is_uninitialized relies on global
PeopleStore being uninitialized which causes flakiness; update the test to not
depend on global state by either (a) explicitly reset or clear the singleton
before assertions (call the test-only reset/clear API on PeopleStore or add one)
or (b) initialize the facade with a fresh, empty PeopleStore instance and then
call get_contact, list_contacts, and lookup_contact to assert their fail‑soft
behavior; locate changes around the test function
contact_facade_fail_softs_when_store_is_uninitialized and the facade helpers
get_contact, list_contacts, and lookup_contact to implement the reset/explicit
initialization.
src/openhuman/memory/ops/files.rs-115-118 (1)

115-118: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use the shared process-wide env lock for OPENHUMAN_WORKSPACE mutations.

The module-local env_mutex only serializes tests in this file. Parallel tests in other modules can still mutate the same env var concurrently and cause flaky behavior.

Proposed fix
-    use std::sync::{Mutex, OnceLock};
+    use crate::openhuman::config::TEST_ENV_LOCK;
@@
-    fn env_mutex() -> &'static Mutex<()> {
-        static ENV_MUTEX: OnceLock<Mutex<()>> = OnceLock::new();
-        ENV_MUTEX.get_or_init(|| Mutex::new(()))
-    }
+    fn env_mutex() -> &'static std::sync::Mutex<()> {
+        &TEST_ENV_LOCK
+    }

Then keep current call sites unchanged (env_mutex().lock()...), now backed by the shared lock.

Also applies to: 148-150, 203-205, 229-231, 253-255, 284-286, 304-306, 329-331, 358-360

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/memory/ops/files.rs` around lines 115 - 118, Replace the
module-local static ENV_MUTEX with the shared process-wide environment lock used
across the test-suite: change fn env_mutex() -> &'static Mutex<()> to delegate
to the global/shared OnceLock<Mutex<()>> (the process-wide env lock provided by
your test helpers or crate-level test utilities) instead of creating a new
static here, so existing call sites like env_mutex().lock() continue to work but
now serialize mutations of OPENHUMAN_WORKSPACE across the whole process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Built-in agents, prompts, orchestration, and agent runtime in src/openhuman/agent/. memory Memory store, memory tree, recall, summarization, and embeddings in src/openhuman/memory/.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant