refactor(tinycortex): W7 — shim memory_tools over the crate tool_memory engine - #4789
Conversation
…ry engine Reduce memory_tools to a thin host shim over tinycortex::memory::tool_memory (store/types/render), completing the W7 long-tail. The rule storage/retrieval engine, the ToolMemoryRule/priority/ source types, and the prompt renderer are all the crate's (a byte-identical port); the host keeps only the surfaces the crate can't own. Sidesteps gap G1 (the host Memory trait's `sqlite_conn` escape hatch is not yet unified with the crate Memory trait) with a small adapter instead of the W3 trait surgery: - store.rs: re-export the crate ToolMemoryStore + TOOL_MEMORY_PROMPT_CAP, and add `tool_memory_store(Arc<dyn host::Memory>)`, which wraps the host memory trait object in HostMemoryBridge — a newtype impl of the crate `Memory` trait forwarding all 11 methods (host = crate + sqlite_conn, and the value types are already crate re-exports, so every method forwards verbatim). - The one exception is `list()`: UnifiedMemory::list() lists *documents* and surfaces each document's title as the entry content, so tool rules stored as JSON can't be round-tripped back through it. The host ToolMemoryStore worked around this with a `sqlite_conn` fast-path reading the real content from `memory_docs`; that fast-path is relocated into the adapter's list() (using the escape hatch the host trait object still exposes), so the crate list_rules deserializes real content. Connectionless backends (test MockMemory) fall back to the faithful trait list(). - types.rs: re-export crate ToolMemoryRule/ToolMemoryPriority/ToolMemorySource/ tool_memory_namespace. - prompt.rs: re-export the crate ToolMemoryRulesSection + render_tool_memory_rules + TOOL_MEMORY_HEADING; keep the host PromptSection impl (a host-local trait implemented for the crate section under the orphan rule). - Repoint the 8 ToolMemoryStore::new(host_memory) call sites (memory/ops, session builder, capture hook, list/put tools) to tool_memory_store(..). - Delete store_tests.rs (engine coverage lives in the crate; the adapter path is exercised end-to-end by capture::tests + the put/list tool tests over a real isolated workspace). Verified: cargo check --lib exit 0; cargo test --lib memory_tools 30 passed (incl. the put->persist->list roundtrip over a real UnifiedMemory workspace, which is what surfaced the list() content gap). Claude-Session: https://claude.ai/code/session_01X39btnEnHSTuPSYYvgyjrb
📝 WalkthroughWalkthroughTool-memory types, prompt rendering, and storage APIs now use tinycortex re-exports. A host memory bridge adapts the local ChangesTool Memory Bridge
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/memory_tools/types.rs (1)
4-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGarbled doc comment wording.
"the
[tool_memory_namespace]helper are the crate's (a byte-identical port," reads as an incomplete sentence — likely a dropped noun (e.g. "the crate's own types").✏️ Suggested fix
-//! [`tool_memory_namespace`] helper are the crate's (a byte-identical port, -//! preserving the serde wire strings + `rule/{id}` storage keys). Host consumers +//! [`tool_memory_namespace`] helper are the crate's own types (a byte-identical +//! port, preserving the serde wire strings + `rule/{id}` storage keys). Host +//! consumers🤖 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/types.rs` around lines 4 - 7, Rewrite the module-level documentation around ToolMemoryRule, ToolMemoryPriority, ToolMemorySource, and tool_memory_namespace into a complete grammatical sentence by supplying the missing noun after “crate’s.” Preserve the existing claims about byte-identical behavior, serde wire strings, rule/{id} storage keys, and unchanged memory_tools::types::* imports.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@src/openhuman/memory_tools/types.rs`:
- Around line 4-7: Rewrite the module-level documentation around ToolMemoryRule,
ToolMemoryPriority, ToolMemorySource, and tool_memory_namespace into a complete
grammatical sentence by supplying the missing noun after “crate’s.” Preserve the
existing claims about byte-identical behavior, serde wire strings, rule/{id}
storage keys, and unchanged memory_tools::types::* imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69f50e2b-ad0b-4c56-8185-83880a8c5bad
📒 Files selected for processing (11)
src/openhuman/agent/harness/session/builder/helpers.rssrc/openhuman/memory/ops/tool_memory.rssrc/openhuman/memory_tools/README.mdsrc/openhuman/memory_tools/capture.rssrc/openhuman/memory_tools/mod.rssrc/openhuman/memory_tools/prompt.rssrc/openhuman/memory_tools/store.rssrc/openhuman/memory_tools/store_tests.rssrc/openhuman/memory_tools/tools/list.rssrc/openhuman/memory_tools/tools/put.rssrc/openhuman/memory_tools/types.rs
💤 Files with no reviewable changes (1)
- src/openhuman/memory_tools/store_tests.rs
Summary
memory_toolsto a thin host shim overtinycortex::memory::tool_memory(store / types / render), completing the W7 long-tail — every hostmemory_*engine now delegates to the crate.ToolMemoryRule/ priority / source types, and the prompt renderer are all the crate's (a byte-identical port); the host keeps only the surfaces the crate can't own.Problem
memory_toolsduplicatedtinycortex::memory::tool_memory, a complete port. Flipping it looked blocked on gap G1 — the crateToolMemoryStore::newtakesArc<dyncrate::Memory>, while host call sites holdArc<dynhost::Memory>(the crate trait plus the host-onlysqlite_conn()escape hatch). Full G1 retirement (droppingsqlite_connfrom the trait) is genuine W3 work: the only othersqlite_connconsumer, theArchivistHook, needs a raw FTS5 connection shared withUnifiedMemory'sunifiedstore, which is part of the W3unifiedre-homing.Solution — a bridge adapter, no trait surgery
The host
Memorytrait is exactly the crate trait plussqlite_conn(verified: identical method sets otherwise), and the value types are already crate re-exports (W2). So a small adapter unblocks the flip without waiting on W3:store.rs— re-export the crateToolMemoryStore+TOOL_MEMORY_PROMPT_CAP, and addtool_memory_store(Arc<dyn host::Memory>). It wraps the host trait object inHostMemoryBridge, a newtypeimplcrate::Memoryforwarding all 11 methods verbatim (no conversion — the types are shared).list().UnifiedMemory::list()lists documents and surfaces each document's title as the entry content, so tool rules stored as JSON inmemory_docscan't be round-tripped back through it — the cratelist_ruleswould fail to deserialize the title and silently drop the rule. The hostToolMemoryStoreworked around this with asqlite_connfast-path reading the real content straight frommemory_docs; that fast-path is relocated into the adapter'slist()(via the escape hatch the host trait object still exposes). Connectionless backends (the testMockMemory) fall back to the faithful traitlist(). (This gap is exactly why memory_tools looked G1-blocked — the fast-path was load-bearing, not a mere optimization.)types.rs— re-exportToolMemoryRule/ToolMemoryPriority/ToolMemorySource/tool_memory_namespace.prompt.rs— re-export the crateToolMemoryRulesSection+render_tool_memory_rules+TOOL_MEMORY_HEADING, and keep the hostPromptSectionimpl (a host-local trait implemented for the crate section under the orphan rule).ToolMemoryStore::new(host_memory)call sites (memory/ops, session builder, capture hook, list/put tools) totool_memory_store(..).store_tests.rs(engine coverage lives in the crate; the adapter path is exercised end-to-end bycapture::tests+ the put/list tool tests over a real isolated workspace).Submission Checklist
list()content gap and now covers the adapter fast-path);capture::testscover the hook overMockMemory; the crate owns the engine's own unit tests.memory_toolstests; CI diff-cover is the gate.docs/tinycortex-*migration plan.Impact
list()fast-path relocation keeps the exact prior read semantics; storage keys, namespaces, wire types, and the prompt block are unchanged.cargo check --libexit 0;cargo test --lib memory_tools→ 30 passed; 0 failed.--no-verify— the hook fails on two environmental gaps unrelated to the diff (app/src-taurican't buildglib-sysfor lack of GTK libs;lint:commands-tokensneedsripgrep). No changed code lives in theapp/src-tauriworld.Related
Memorytrait with the crate's by re-plumbing theArchivistHook's sharedunified-store connection) remains W3 — this PR does not need it.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check— noapp/srcchangespnpm typecheck— no frontend changescargo test --lib memory_tools→ 30 passed;cargo check --libexit 0cargo fmt --checkclean on changed files;cargo check --libexit 0app/src-tauribuild blocked by missing system GTK libs (env, not diff)Validation Blocked
command:cargo check --manifest-path app/src-tauri/Cargo.tomlerror:glib-sysbuild fails (missing GTK/glib system libs)impact:environmental only — no changed code in theapp/src-tauriworldBehavior Changes
list()-over-memory_docsread semantics.https://claude.ai/code/session_01X39btnEnHSTuPSYYvgyjrb
Summary by CodeRabbit
Refactor
Documentation
Bug Fixes