refactor(memory): confine raw profile SQLite, guard the tool_memory tools, collapse three re-export shims - #5470
Conversation
Co-authored-by: Medulla <medulla@tinyhumans.ai>
…rd and binding Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Delete src/openhuman/memory/diff/types.rs, which was a 19-line pub-use of tinycortex::memory::diff. Importers now name the crate module directly. Pure re-point: type identity is unchanged. Co-authored-by: Medulla <medulla@tinyhumans.ai>
memory::goals re-exported the crate goal types under a second spelling; every external caller already named tinycortex_api::goals directly. The one internal consumer (ops.rs) now does too, so the host converges on a single path. No type or signature change. Co-authored-by: Medulla <medulla@tinyhumans.ai>
The host store was six async wrappers, each engine::f(..) plus map_err(to_string). Call sites (ops, tools, enrich, the embedded driver) now call tinycortex::memory::goals::store directly and carry the same to_string() mapping inline, so error text and control flow are byte-identical; the wrappers were never async in substance, so dropping .await changes nothing observable. Mapping the now-typed engine MemoryError onto the contract's Invalid/NotFound is a behaviour change and stays a follow-up; the driver's module docs record that. Co-authored-by: Medulla <medulla@tinyhumans.ai>
memory::conversations re-exported 16 crate items under a host path. All ~25 consumers now name tinycortex::memory::conversations directly, so the host module is only what it actually owns: the event-bus persistence subscriber and the spawn_blocking wrappers (tinyhumansai#5156), both of which stay. Pure re-point; no signature or type change. Co-authored-by: Medulla <medulla@tinyhumans.ai>
When a tool name is not provided in the put request, the operation now returns an appropriate error instead of panicking or producing undefined behavior. This ensures the API remains robust against incomplete input. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The put tool was inadvertently removed from the tool memory module, breaking the ability to store new entries. This change restores the tool's implementation, ensuring that put operations function correctly again. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The put tool was calling `active_memory_client` from the `ops` module, but the function has been moved to the `helpers` module. This change updates the import path so the tool can find and use the correct function. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The put tool now uses the active memory guard and calls `put_tool_rule`, which returns unit instead of the stored rule. To preserve the tool's contract of returning the stored rule, the rule is read back by its generated id after insertion, ensuring the response includes the normalised tool name and refreshed timestamps. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…attern match The SQL query in the profile store was using a LIKE operator with a key pattern, but the parameter being passed is an exact key value, not a pattern. Changed the operator to an equality check to ensure the query correctly matches the exact key rather than interpreting it as a pattern, which could lead to incorrect or missed matches. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Changed the key comparison in the profile existence check from an exact match to a LIKE pattern match, ensuring that the query correctly handles key patterns that may contain wildcards or partial matches as intended by the surrounding logic. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The lockfile now includes the tinybus and tinybus-macros packages as dependencies of the main application, while removing the unused cmake crate and an unnecessary indexmap dependency from serde_json. This reflects the addition of the tinybus event bus library to the project. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The `for_tests` constructor was gated behind `#[cfg(test)]`, which made it invisible to integration tests in `tests/` because those link the library compiled without `cfg(test)`. The attribute is replaced with `#[doc(hidden)]` so the method is always compiled and linkable, while still being kept out of the public documentation. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…on tests Update the integration test harness and a standalone test to pass a ProfileStore instance to FacetCache instead of a raw connection, ensuring the cache uses the store layer that will be required by the production code path. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Remove the now-unused `crate::core::bus::BUS` imports across unit and integration tests, as the bus is no longer referenced directly in these test modules. This cleans up dead imports and reduces noise in the test code. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
This reverts commit 62067fa. Co-authored-by: Medulla <medulla@tinyhumans.ai>
Moved the `ProfileStore` import after the `profile` submodule imports to follow Rust's convention of importing parent modules after their children, resolving a compiler warning about out-of-order imports. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds a typed ChangesMemory boundary and ownership migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ToolHandler
participant MemoryGuard
participant ToolMemoryStore
ToolHandler->>MemoryGuard: request tool-memory capability
MemoryGuard->>ToolMemoryStore: enforce tier and write rule
ToolMemoryStore-->>MemoryGuard: return generated rule identifier
MemoryGuard-->>ToolHandler: return normalized persisted rule
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
memory
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/security/credentials/ops.rs (1)
488-488: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOffload conversation purges from async auth flows.
Both changed call sites invoke synchronous TinyCortex conversation deletion inside async authentication/revalidation paths. A large pre-login workspace can block the executor and slow login/revalidation. Use an existing blocking wrapper such as
memory::conversations::blocking::purge_threadsand await it before continuing.
src/openhuman/security/credentials/ops.rs#L488-L488: offload the first-login purge before writing session storage.src/openhuman/desktop/app_state/ops.rs#L514-L514: offload the pending-session revalidation purge before returning the reloaded configuration.🤖 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/security/credentials/ops.rs` at line 488, Offload the synchronous conversation purge from both async authentication flows: in src/openhuman/security/credentials/ops.rs#L488-L488, replace conversations::purge_threads in the first-login path with the existing blocking wrapper and await it before writing session storage; apply the same change in src/openhuman/desktop/app_state/ops.rs#L514-L514 for pending-session revalidation, awaiting completion before returning the reloaded configuration.
🧹 Nitpick comments (4)
src/openhuman/memory/goals/tools.rs (1)
50-50: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep synchronous goals storage off tool executor tasks.
Line 50, Line 99, Line 151, and Line 199 call synchronous TinyCortex storage from async tool methods. A slow filesystem or mutation-lock wait can block the agent tool loop. Run the store operation through a blocking boundary and preserve the current
ToolResultmapping. The published goals store documents synchronous filesystem operations and serialized mutations; verify the locked0.1.0API before merging. (docs.rs)Also applies to: 99-99, 151-151, 199-199
🤖 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/goals/tools.rs` at line 50, Update the async tool methods containing the storage calls at load, create, update, and delete to execute TinyCortex store operations through an async blocking boundary, preventing synchronous filesystem or mutation-lock work from running on the tool executor. Verify the locked TinyCortex 0.1.0 API and preserve each method’s existing ToolResult success and error mapping.src/openhuman/memory/goals/ops.rs (1)
35-37: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep synchronous goals storage off async RPC tasks.
Line 35, Line 42, Line 59, Line 66, Line 92, and Line 104 call synchronous TinyCortex storage from async RPC functions. Concurrent mutations can also wait on the process-wide store lock. Use one blocking helper for these calls and keep the existing
RpcOutcomeand error mappings. The published goals store documents synchronous filesystem operations and serialized mutations; verify the locked0.1.0API before merging. (docs.rs)Also applies to: 42-42, 59-60, 66-67, 92-92, 104-104
🤖 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/goals/ops.rs` around lines 35 - 37, Move the synchronous TinyCortex storage calls in the affected async RPC functions into a shared blocking helper, covering load and mutation operations at the referenced call sites. Preserve each function’s existing RpcOutcome construction and error mapping, and use the locked TinyCortex 0.1.0 API while ensuring concurrent mutations remain serialized without blocking async RPC tasks.src/openhuman/memory/driver/embedded/goals.rs (2)
56-56: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep goal storage off the async executor.
Line 56 and Line 67 call synchronous TinyCortex file APIs from async
MemoryGoalsmethods. A slow filesystem or the process-wide mutation lock can block a runtime worker and delay unrelated memory requests. Use the existing blocking boundary orspawn_blockingfor both calls, then map the result tohost_error. The published TinyCortex goals store documents synchronous filesystem operations and a process-wide mutation lock; verify the locked0.1.0API before merging. (docs.rs)Also applies to: 67-67
🤖 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/driver/embedded/goals.rs` at line 56, Move both synchronous TinyCortex calls in the async MemoryGoals methods, including store::load at line 56 and the call at line 67, behind the existing blocking boundary or spawn_blocking. Await the blocking operation, then preserve the current result mapping to host_error for failures.
56-67: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove synchronous goals-store I/O off async tasks.
The goals API currently calls TinyCortex
store::load,store::save,add,edit, anddeletesynchronously fromasync fnpaths. Run these calls through a shared blocking boundary, then preserve each caller’s error/response mapping. This addresses the embedded driver, enrichment load, list/add/edit/delete RPCs, reflection fallback loads, and tool callers.🤖 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/driver/embedded/goals.rs` around lines 56 - 67, The synchronous TinyCortex goals-store operations must not run directly on async tasks. Route store::load/save/add/edit/delete through the shared blocking boundary while preserving each caller’s existing error and response mapping: embedded driver goals load/set_goals in src/openhuman/memory/driver/embedded/goals.rs:56-67; enrichment loading in src/openhuman/memory/goals/enrich.rs:69-69; goals RPC operations in src/openhuman/memory/goals/ops.rs:35-66 and 92-104; and tool callers in src/openhuman/memory/goals/tools.rs:50-52, 99-102, 151-154, and 199-202.
🤖 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.
Inline comments:
In `@docs/specs/memory-guard-allowlist.md`:
- Around line 118-120: Update the confinement statement in
memory-guard-allowlist.md to limit compiler-enforced coverage to the
profile_conn() and ProfileStore paths. Retain
agent/harness/archivist/lifecycle.rs::profile::profile_upsert as a documented
uncovered exception until it is removed or incorporated into the guard model,
and adjust the nearby test reference wording to match.
In `@src/openhuman/memory/conversations/mod.rs`:
- Around line 17-18: Restore the public re-export in the conversations module by
adding register_conversation_persistence_subscriber from the private bus module
to its public API. Keep bus private and expose the existing function through
memory::conversations.
In `@src/openhuman/memory/guard/audit.rs`:
- Around line 137-161: Add a per-operation correlation value to the audit
recorder and propagate it through the refusal publication and query paths,
updating `watermark`/`denied_for_since` or their callers so results match both
the driver ID and the originating operation. Ensure parallel tests sharing a
driver ID cannot observe or satisfy assertions with another operation’s records.
---
Outside diff comments:
In `@src/openhuman/security/credentials/ops.rs`:
- Line 488: Offload the synchronous conversation purge from both async
authentication flows: in src/openhuman/security/credentials/ops.rs#L488-L488,
replace conversations::purge_threads in the first-login path with the existing
blocking wrapper and await it before writing session storage; apply the same
change in src/openhuman/desktop/app_state/ops.rs#L514-L514 for pending-session
revalidation, awaiting completion before returning the reloaded configuration.
---
Nitpick comments:
In `@src/openhuman/memory/driver/embedded/goals.rs`:
- Line 56: Move both synchronous TinyCortex calls in the async MemoryGoals
methods, including store::load at line 56 and the call at line 67, behind the
existing blocking boundary or spawn_blocking. Await the blocking operation, then
preserve the current result mapping to host_error for failures.
- Around line 56-67: The synchronous TinyCortex goals-store operations must not
run directly on async tasks. Route store::load/save/add/edit/delete through the
shared blocking boundary while preserving each caller’s existing error and
response mapping: embedded driver goals load/set_goals in
src/openhuman/memory/driver/embedded/goals.rs:56-67; enrichment loading in
src/openhuman/memory/goals/enrich.rs:69-69; goals RPC operations in
src/openhuman/memory/goals/ops.rs:35-66 and 92-104; and tool callers in
src/openhuman/memory/goals/tools.rs:50-52, 99-102, 151-154, and 199-202.
In `@src/openhuman/memory/goals/ops.rs`:
- Around line 35-37: Move the synchronous TinyCortex storage calls in the
affected async RPC functions into a shared blocking helper, covering load and
mutation operations at the referenced call sites. Preserve each function’s
existing RpcOutcome construction and error mapping, and use the locked
TinyCortex 0.1.0 API while ensuring concurrent mutations remain serialized
without blocking async RPC tasks.
In `@src/openhuman/memory/goals/tools.rs`:
- Line 50: Update the async tool methods containing the storage calls at load,
create, update, and delete to execute TinyCortex store operations through an
async blocking boundary, preventing synchronous filesystem or mutation-lock work
from running on the tool executor. Verify the locked TinyCortex 0.1.0 API and
preserve each method’s existing ToolResult success and error mapping.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08579b46-69d6-4011-a082-f6f7c03b2bf6
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (130)
docs/specs/memory-guard-allowlist.mdsrc/core/bus.rssrc/core/jsonrpc.rssrc/core/mod.rssrc/openhuman/agent/artifacts/store_tests.rssrc/openhuman/agent/bus.rssrc/openhuman/agent/harness/session/runtime_tests.rssrc/openhuman/agent/harness/session/turn/tools.rssrc/openhuman/agent/harness/subagent_runner/ops/graph.rssrc/openhuman/agent/learning/README.mdsrc/openhuman/agent/learning/cache.rssrc/openhuman/agent/learning/cache_tests.rssrc/openhuman/agent/learning/extract/signature.rssrc/openhuman/agent/learning/profile_md_renderer.rssrc/openhuman/agent/learning/prompt_sections.rssrc/openhuman/agent/learning/prompt_sections_tests.rssrc/openhuman/agent/learning/schemas.rssrc/openhuman/agent/learning/stability_detector.rssrc/openhuman/agent/learning/startup.rssrc/openhuman/agent/learning/tools.rssrc/openhuman/agent/orchestration/run_ledger_finalize.rssrc/openhuman/agent/orchestration/run_ledger_finalize_tests.rssrc/openhuman/agent/orchestration/tools/spawn_async_subagent.rssrc/openhuman/agent/orchestration/tools/spawn_subagent.rssrc/openhuman/agent/orchestration/tools/spawn_worker_thread.rssrc/openhuman/agent/orchestration/tools/tools_e2e_tests.rssrc/openhuman/agent/orchestration/tools/worker_thread.rssrc/openhuman/agent/task_session.rssrc/openhuman/agent/tinyagents/tools.rssrc/openhuman/agent/triage/escalation.rssrc/openhuman/agent/triage/evaluator.rssrc/openhuman/agent/triage/events.rssrc/openhuman/channels/bus.rssrc/openhuman/channels/host/adapters.rssrc/openhuman/channels/proactive.rssrc/openhuman/channels/providers/telegram/approval_surface.rssrc/openhuman/channels/providers/telegram/approval_surface_tests.rssrc/openhuman/channels/providers/telegram/bus.rssrc/openhuman/channels/providers/telegram/bus_tests.rssrc/openhuman/channels/providers/telegram/remote_control.rssrc/openhuman/channels/routes_tests.rssrc/openhuman/channels/runtime/dispatch/processor.rssrc/openhuman/channels/runtime/test_support.rssrc/openhuman/channels/tests/health.rssrc/openhuman/channels/tests/runtime_dispatch.rssrc/openhuman/config/ops/agent.rssrc/openhuman/cron/bus.rssrc/openhuman/cron/scheduler_tests.rssrc/openhuman/desktop/app_state/ops.rssrc/openhuman/desktop/notifications/bus.rssrc/openhuman/flows/bus.rssrc/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rssrc/openhuman/inference/provider/factory_tests.rssrc/openhuman/inference/provider/openhuman_backend_model.rssrc/openhuman/inference/provider/ops/http_error.rssrc/openhuman/inference/provider/ops_tests.rssrc/openhuman/integrations/composio/ops/direct_mode.rssrc/openhuman/integrations/task_sources/bus.rssrc/openhuman/meet/backend_bot/calendar.rssrc/openhuman/memory/agent/memory_loader.rssrc/openhuman/memory/binding.rssrc/openhuman/memory/bypass_allowlist_tests.rssrc/openhuman/memory/conversations/blocking.rssrc/openhuman/memory/conversations/bus.rssrc/openhuman/memory/conversations/mod.rssrc/openhuman/memory/diff/mod.rssrc/openhuman/memory/diff/ops.rssrc/openhuman/memory/diff/rpc.rssrc/openhuman/memory/diff/tools.rssrc/openhuman/memory/diff/types.rssrc/openhuman/memory/driver/embedded/diff.rssrc/openhuman/memory/driver/embedded/goals.rssrc/openhuman/memory/global.rssrc/openhuman/memory/goals/enrich.rssrc/openhuman/memory/goals/mod.rssrc/openhuman/memory/goals/ops.rssrc/openhuman/memory/goals/store.rssrc/openhuman/memory/goals/tools.rssrc/openhuman/memory/guard/audit.rssrc/openhuman/memory/guard/mod.rssrc/openhuman/memory/guard/provider_tests.rssrc/openhuman/memory/ops/sync.rssrc/openhuman/memory/ops/tool_memory.rssrc/openhuman/memory/store/client.rssrc/openhuman/memory/store/client_tests.rssrc/openhuman/memory/store/mod.rssrc/openhuman/memory/store/profile_store.rssrc/openhuman/memory/store/profile_store_tests.rssrc/openhuman/memory/sync/composio/bus.rssrc/openhuman/memory/sync/composio/providers/profile.rssrc/openhuman/memory/sync_events.rssrc/openhuman/memory/sync_pipeline_e2e_tests.rssrc/openhuman/memory/tinycortex/sync.rssrc/openhuman/memory/tool_memory/tools/list.rssrc/openhuman/memory/tool_memory/tools/put.rssrc/openhuman/memory/tree/tree_runtime/bus.rssrc/openhuman/security/approval/gate.rssrc/openhuman/security/credentials/bus.rssrc/openhuman/security/credentials/ops.rssrc/openhuman/security/credentials/session_support.rssrc/openhuman/security/devices/bus.rssrc/openhuman/security/egress/emit_tests.rssrc/openhuman/security/keyring_consent/policy.rssrc/openhuman/skills/bus.rssrc/openhuman/skills/ops_create.rssrc/openhuman/skills/webhooks/bus.rssrc/openhuman/subconscious/profiles/memory.rssrc/openhuman/subconscious/profiles/memory_tests.rssrc/openhuman/subconscious/session.rssrc/openhuman/subconscious/user_thread.rssrc/openhuman/threads/ops.rssrc/openhuman/threads/ops_tests.rssrc/openhuman/threads/welcome_migration.rssrc/openhuman/tools/ops.rssrc/openhuman/voice/bus.rssrc/openhuman/web_chat/event_bus.rstests/agent_harness_e2e.rstests/calendar_grounding_e2e.rstests/composio_list_tools_stack_overflow_regression.rstests/config_auth_app_state_connectivity_e2e.rstests/json_rpc_e2e.rstests/learning_phase4_integration_test.rstests/monitor_agent_e2e.rstests/personality_e2e.rstests/raw_coverage/memory_core_threads_raw_coverage_e2e.rstests/subconscious_conversation_e2e.rstests/subconscious_fullstack_e2e.rstests/subconscious_triggers_e2e.rstests/transcript_search_e2e.rs
💤 Files with no reviewable changes (2)
- src/openhuman/memory/diff/types.rs
- src/openhuman/memory/goals/store.rs
| SQL statement against `user_profile` is inside the memory family, and the | ||
| compiler enforces that; `client_tests.rs::profile_conn_is_confined_to_the_memory_family` | ||
| restates the rule in a form that names the offending file. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Narrow the profile-confinement claim.
Line 118 says every user_profile SQL statement is inside src/openhuman/memory. Line 142 documents agent/harness/archivist/lifecycle.rs::profile::profile_upsert writing through an injected connection outside that family. The document is internally inconsistent, even if that path is currently inert.
State that compiler-enforced confinement applies to the profile_conn() and ProfileStore paths. Keep profile_upsert documented as an uncovered exception until it is removed or included in the guard model.
Suggested wording
-Every SQL statement against `user_profile` is inside the memory family, and the compiler enforces that;
+SQL reached through `profile_conn()` and `ProfileStore` stays inside the memory family, and the compiler enforces that for this path;🤖 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 `@docs/specs/memory-guard-allowlist.md` around lines 118 - 120, Update the
confinement statement in memory-guard-allowlist.md to limit compiler-enforced
coverage to the profile_conn() and ProfileStore paths. Retain
agent/harness/archivist/lifecycle.rs::profile::profile_upsert as a documented
uncovered exception until it is removed or incorporated into the guard model,
and adjust the nearby test reference wording to match.
| pub mod blocking; | ||
| mod bus; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore the persistence-subscriber re-export.
bus is private. This module no longer exposes register_conversation_persistence_subscriber. Host startup code therefore cannot access the retained persistence integration through memory::conversations.
Add pub use bus::register_conversation_persistence_subscriber;.
🤖 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/conversations/mod.rs` around lines 17 - 18, Restore the
public re-export in the conversations module by adding
register_conversation_persistence_subscriber from the private bus module to its
public API. Keep bus private and expose the existing function through
memory::conversations.
| /// How many refusals have been recorded so far, process-wide. | ||
| /// | ||
| /// Take this before driving the code under test and pass it to | ||
| /// [`denied_for_since`] — sibling tests run in parallel, reuse the same | ||
| /// driver ids, and must not see each other's rows. | ||
| pub(crate) fn watermark() -> usize { | ||
| DENIED.lock().map(|log| log.len()).unwrap_or(0) | ||
| } | ||
|
|
||
| /// Refusals for `driver_id` recorded at or after `watermark`. | ||
| /// Non-draining: the log is shared, so nothing may consume from it. | ||
| pub(crate) fn denied_for_since( | ||
| watermark: usize, | ||
| driver_id: &str, | ||
| ) -> Vec<(String, String, String)> { | ||
| DENIED | ||
| .lock() | ||
| .map(|log| { | ||
| log.iter() | ||
| .skip(watermark) | ||
| .filter(|(id, _, _)| id == driver_id) | ||
| .cloned() | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate audit records per test operation.
Lines 139-161 do not isolate parallel test activity when sibling tests use the same driver ID. A sibling can append a matching denial after the watermark and before denied_for_since. The refusal test can then pass without its own operation publishing. The success test can also fail from an unrelated denial.
Add a per-operation correlation value to the recorder and query it. Alternatively, serialize tests that inspect this process-wide recorder.
🤖 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/guard/audit.rs` around lines 137 - 161, Add a
per-operation correlation value to the audit recorder and propagate it through
the refusal publication and query paths, updating `watermark`/`denied_for_since`
or their callers so results match both the driver ID and the originating
operation. Ensure parallel tests sharing a driver ID cannot observe or satisfy
assertions with another operation’s records.
# Conflicts: # src/openhuman/agent/triage/escalation.rs # src/openhuman/agent/triage/events.rs # src/openhuman/inference/provider/factory_tests.rs # src/openhuman/inference/provider/ops_tests.rs # src/openhuman/memory/guard/audit.rs # src/openhuman/memory/guard/provider_tests.rs
|
Merged Four files conflicted, all in the Post-merge, everything that was previously red is green:
The six failures I had classified as pre-existing were indeed upstream's, and upstream's own repairs cleared them — so the "pre-existing" column in the PR description is now stale in this PR's favour. |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0394 · 394,087 in / 139,242 out · 329,629 cached (84%) · z-ai/glm-5.2
critique: $0.0267 · 154,358 in / 106,200 out · 131,371 cached (85%) · z-ai/glm-5.2
security: $0.0078 · 131,963 in / 22,660 out · 112,822 cached (85%) · z-ai/glm-5.2
tests: $0.0014 · 36,167 in / 2,186 out · 28,919 cached (80%) · z-ai/glm-5.2
description: $0.0022 · 37,911 in / 5,472 out · 28,311 cached (75%) · z-ai/glm-5.2
| self.store.list_all() | ||
| } | ||
|
|
||
| /// List active facets belonging to a specific class. |
There was a problem hiding this comment.
Update the class-filter list method to use store
The class-filter method between list_all and get is not in the diff, so it is unchanged. Its body previously used self.conn (the only field the old struct had), but the struct now has store: ProfileStore and no conn field. The unchanged method therefore references a field that no longer exists and will not compile. It needs to be converted to self.store.list_active_by_class(...) (or the equivalent ProfileStore method) like the siblings above and below it.
[RULE] untrusted-repo-rules ·
| ), | ||
| ]; | ||
| for (facet_id, key, value) in rows { | ||
| store |
There was a problem hiding this comment.
Seeded facets use Workflow but the legacy oracle filters facet_type = 'skill'
The seeded rows are inserted with FacetType::Workflow, but legacy_like_query filters WHERE facet_type = 'skill'. On this seed data that legacy query can never match, so it returns false for every case. skill_identity_matches_agrees_with_the_legacy_like_query then asserts store.skill_identity_matches(...) == legacy for all cases, and immediately afterward asserts store.skill_identity_matches("skill:%:%:email", "user@example.com") is true. Those cannot both hold: if skill_identity_matches also filters by facet_type = 'skill' it returns false and the non-vacuity assertion fails; if it does not (e.g. it only matches on the key prefix), it returns true on the cross-toolkit case where legacy is false and the equality assertion fails. The seed should almost certainly insert these rows as FacetType::Skill, not FacetType::Workflow, so that the legacy 'skill' filter can actually hit.
**[RULE] ** ·
|
|
||
| use crate::openhuman::agent::task_board::TaskBoardCard; | ||
| use crate::openhuman::memory::conversations::{ | ||
| use tinycortex::memory::conversations::{ |
There was a problem hiding this comment.
Dropped openhuman segment when switching to external crate path
The import path was changed from crate::openhuman::memory::conversations to tinycortex::memory::conversations, dropping the openhuman segment. Since this file lives under src/openhuman/agent/, the crate's module tree almost certainly still has openhuman as a parent module, meaning the public path would be tinycortex::openhuman::memory::conversations (if the external-name self-reference is even intended). The new path as written likely fails to resolve, or resolves to a different item than the original import. If the intent was to switch from the internal crate:: prefix to the external crate name, the openhuman segment should still be present.
[RULE] Report only problems this pull request introduces. ·
| pub mod blocking; | ||
| mod bus; | ||
|
|
||
| pub use bus::register_conversation_persistence_subscriber; |
There was a problem hiding this comment.
Removing the pub use re-exports breaks any caller still on the old import path
The diff removes the pub use tinycortex::memory::conversations::{ … } re-export that the prior module doc explicitly said existed so that "the ~30 host consumers … keep their import paths … unchanged." If any caller still imports these types through openhuman::memory::conversations::* (or through openhuman::memory's own re-export of this module), removing the pub use will break compilation. I can only see this one file, so I cannot confirm whether every consumer was migrated to name tinycortex::memory::conversations directly as the new doc claims; if any were missed, this change breaks them.
**[RULE] ** ·
| @@ -511,7 +511,7 @@ async fn activate_revalidated_user_dir(user_id: &str) -> Result<Config, String> | |||
| ); | |||
| if previous_active.is_none() { | |||
| let pre_ws = crate::openhuman::config::pre_login_user_dir(&root_dir).join("workspace"); | |||
There was a problem hiding this comment.
Verify tinycortex crate path resolves and matches adjacent internal path
The diff changes the crate path for purge_threads from crate::openhuman::memory::conversations to tinycortex::memory::conversations, but the immediately preceding line still accesses config via the internal path crate::openhuman::config::pre_login_user_dir. If memory has been extracted into an external tinycortex crate while config remained in the internal openhuman module tree, the two-line span is consistent and this is fine. But if tinycortex is not a declared dependency, or if memory still lives under crate::openhuman::, this change will not compile. The repository's own rules consistently reference src/openhuman/ module paths and never mention a tinycortex crate, which makes the path switch suspect. At minimum the adjacent-line inconsistency should be confirmed: the author should verify that tinycortex::memory::conversations::purge_threads resolves and that the mismatch with crate::openhuman::config on the line above is intentional rather than a half-applied refactor.
[RULE] correctness ·
|
|
||
| use super::source::ChunkStoreItemSource; | ||
| use super::types::*; | ||
| use tinycortex::memory::diff::types::*; |
There was a problem hiding this comment.
Import switched from local super::types to external crate path
The import is changed from the local module (super::types, i.e. openhuman::memory::diff::types) to an external crate path (tinycortex::memory::diff::types). If the local types module defines or re-exports types that differ from — or are a superset of — what tinycortex provides, ops.rs will either fail to compile or silently use distinct types that its callers (which still reference openhuman::memory::diff::types through super::types) do not expect. Even when names match, two types from different crates are not interchangeable in Rust, so any function in ops.rs that returns or accepts these types would be returning a different type than the rest of the openhuman::memory::diff module expects. This is only safe if super::types was a pure re-export of tinycortex::memory::diff::types, which the diff does not establish.
[RULE] ops.rs handlers return RpcOutcome<T> and delegate from schemas.rs. ·
What this change touches63 files, +889 -404 across 4 components. The code graph knows nothing about these files yet — normal for newly added files, and a cold index otherwise. flowchart LR
n0["src/openhuman<br/>56 files +817 -364<br/>6 findings"]:::blocking
n1["docs/specs<br/>1 file +58 -24"]:::changed
n2["tests<br/>5 files +13 -13"]:::changed
n3["tests/raw_coverage<br/>1 file +1 -3"]:::changed
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
the
priority: p1
…e\n\nrefactor(memory): confine raw profile SQLite, guard the tool_memory tools, collapse three re-export shims\n
1 participant
What
The follow-up pass to #5446, closing the three items that PR explicitly deferred.
1.
profile_conn— raw SQLite confined behind a typedProfileStoreMemoryClient::profile_conn()handed out anArc<Mutex<rusqlite::Connection>>to threedomains outside the memory family, two of which wrote SQL inline at the call site. New
memory/store/profile_store.rsis now the only typed door ontouser_profile; every one ofthe 11 non-test call sites (
agent/learning/{startup,tools,schemas}.rs,memory/sync/composio/providers/profile.rs) is re-pointed, andprofile_conn()is narrowedfrom
pub(crate)topub(in crate::openhuman::memory)with exactly one caller left —profile_store()'s own body.FacetCachesurvives as a learning-side newtype becauseFacetClassis agent vocabulary that must not migrate into the memory family.Read this before believing the title. This is a confinement win, not a guard win.
The profile/facet tables have no capability family in the
tinycortex_apicontract, so readsand writes through
ProfileStorestill run beneath all seven ofMemoryGuard's policy steps:no tier check, no source-scope predicate, no taint stamping, no redaction, no budget, no audit
event. What changed is the door's shape — raw SQLite reachable from three domains became one
typed store whose confinement the compiler enforces. Actually putting profile data under
policy needs a fourteenth capability family in
vendor/tinycortex(out of scope here), or aGuardPolicyhalf-measure that would make thereadonlytier start rejecting the boot-timelearning rebuild loop — a behaviour change that deserves its own PR. The module docs and
docs/specs/memory-guard-allowlist.mdsay this in the same words; please don't let the diffstat talk you out of it.
Related, also not fixed here:
user_profilehas no taint column, so provider-sourcedidentities still land untainted.
2.
tool_memoryagent tools routed through the guardmemory/tool_memory/tools/{list,put}.rswent viaactive_memory_client()+tool_memory_store(client.memory_handle()). They now resolveactive_memory_guard()and gothrough
as_tool_memory().listis 1:1 —memory::tool_memory::ToolMemoryRuleistinycortex_api::tool_memory::ToolMemoryRule, so same rules, same order, same serialization.putneeds a read-back after the write because the contract method returns unit while thetool answers with the stored rule; that read-back is exact, not lossy, and a concurrent
delete in the window errors rather than fabricating a rule.
Two intended behaviour changes, both consequences of actually being guarded:
memory_tools_putnow passes throughadmit_write→SecurityPolicy::enforce_write_tier,so it is refused under the
readonlyautonomy tier. It previously succeeded.Capability::ToolMemory.tool_memory_store(memory_handle())worked over anyArc<dyn Memory>; against anon-advertising driver these two tools now error where they used to function.
The four
ops/tool_memory.rshandlers (tool_rule_put/get/*_json/*_for_prompt) are notre-pointed — they have no contract twin — and their allowlist entries stay with their existing
reasons.
3. M8c — three re-export shims collapsed
Host-only import rewrites, no crate change and no gitlink bump: the
memory_difftypes shim,the goals
GoalsDoc/GoalItemshim,goals/store.rsonto the crate engine, and theconversations type/function shim. Pure relocation — the dropped
asyncon the goals wrappersoffloads nothing that was offloaded before (they were bare
engine::f(..).map_err(to_string),no
spawn_blocking), and error text is preserved at every call site.Expect
grep -rn "tinycortex::" src/openhuman/to go up. Deleting apub useshimre-points importers at
tinycortex::directly. That is the shim leaving, not couplingarriving.
Bypass ratchet
bypass_allowlist_tests.rs: 52 → 49 entries. −4.profile_conn(, −4 tool_memory,+4
.profile_store(, +1client.rs .profile_conn((kept so the needle stays live and afuture re-widening is still watched). The profile slice is net +1 on its own — that is the
honest accounting of a confinement-not-policy change, and the module docs say so. All six lint
tests green;
grep "out of scope for M4\|deferred to M5"oversrc/anddocs/is now empty.Two things about the merge base you should know
origin/maindoes not compile its own lib tests.394fabb92left thecore::event_bus→core::busmigration half-done:memory/guard/audit.rs,memory/binding.rsandmemory/guard/provider_tests.rsstill named the deleted module.Nothing on this branch could be verified until that was fixed, so
f14819bc6does it. Thetwo provider tests observed denials over a
raw_receiver()that no longer exists; since theglobal
BUSis a no-op undercargo test,audit.rsgains a#[cfg(test)]recorder seamwith a watermark, preserving both assertions.
9836f679ais a 69-filecargo fmtof the merge base, which was unformatted. It isisolated in its own commit so the substantive diff stays readable — skip it in review.
e13f7b1a9likewise only regenerates the staleapp/src-tauri/Cargo.lock.Verification
Two independent adversarial passes ran over this branch. Both found the same single blocker —
tests/learning_phase4_integration_test.rsstopped compiling whenFacetCache::newchangedshape, invisible to
cargo test --liband to CI Lite's changed-file lane — which is fixedhere (
ProfileStore::for_testswidened from#[cfg(test)]to#[doc(hidden)] pub, becauseintegration tests link the lib without
cfg(test)).cargo check(lib)cargo check --testscargo test --test learning_phase4_integration_testcargo test --lib openhuman::memorycargo test --lib core::cargo check --no-default-features(slim)scripts/check-kernel-floor.shcargo fmt --check(root +vendor/tinycortex)Every failure above was reproduced byte-identically on an
origin/mainscratch worktree(with
f14819bc6cherry-picked, since main otherwise cannot build its lib tests at all — themost favourable possible reading for main).
guard_explicit_scope_argument_wins_over_the_ambient_onefails on main with the same panic, and
families.rs/families_tests.rs/policy.rsandthe
vendor/tinycortexgitlink are byte-identical to main on this branch.cargo check --manifest-path app/src-tauri/Cargo.tomlfails on both this branch and main(
E0432: unresolved import openhuman_core::core::event_businapp/src-tauri/src/whatsapp_data/mod.rs) — the shell has the same half-done migration. Thisbranch does not fix it; flagging it because it means the desktop shell is currently broken on
main.Non-vacuity was proved for the new tests by breaking the production code and observing the
failure: reverting
put.rs::executeto its pre-branch form made the readonly-tier test failand fired the bypass ratchet naming both needles; flipping
key LIKE ?2tokey = ?2madethe legacy-SQL oracle test fail on
pattern="skill:gmail:%:email".Not in this PR
store/namespace_store/(~11.3k LOC) — 55% of all remaining movable mass and the only itemneeding a real schema/migration port into the crate. Its golden-parity harness pins table
names via hardcoded
&'static [&str]constants, so the constants and the schema must notchange in the same commit or the test passes vacuously. Own sandwich.
agent/harness/archivist/lifecycle.rswritesuser_profileviaprofile::profile_upserton an injected connection and is covered by neither bypass needle. Inert today (no
production
ArchivistHook::new). Aprofile::profile_upsert(needle would surface it forone allowlist entry — worth a follow-up.
Note on the history
The auto-commit hook fires in this worktree and checkpointed several deliberate
break-then-restore edits made while proving test non-vacuity (
50ee06732…f65e8aeb2, and62067fae9/17b775513). Net source effect is zero —git diffbetween the pairs is empty —but they are churn. Squash-merge.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests