feat(flows): host adapter for the memory node — in-graph memory read + flow-scoped write - #5227
Conversation
…er teaching Implements the host side of the tinyflows `memory` node (PR2, tracking issue tinyhumansai#5226): `OpenHumanMemory` in a new `src/openhuman/tinyflows/memory_adapter.rs` implements `tinyflows::caps::MemoryProvider`, wired into `build_capabilities`. - recall/search (scope user|flow|flows) and remember/forget (scope flow only, hard-refused otherwise as defense-in-depth) reuse the existing `enforce_node_tier_gate` / `gate_call_for_tier` permission path — no new gate. - scope "flow" reads/writes the SAME flow_<id> namespace flow_memory_recall / flow_memory_remember use; scope "flows" delegates to a newly-extracted `flows::cross_flow_recall` shared by both surfaces. - flavour delegates to a newly-extracted `memory::tools::flavour::lookup_flavour` shared with MemoryFlavourTool; people delegates to the same people::rpc op people_list uses. - node_contracts.rs gains the memory kind's host overlay; the 12->13 kind-count drift tests (node_contracts, propose_workflow description, list_node_kinds) are updated in lockstep. - workflow_builder prompt.md teaches the memory node (operations, scope rules, canonical dedupe pattern) and corrects the now-stale "workflows can never write memory" claim; matching builder_prompt.rs guard tests updated. - vendor/tinyflows gitlink bumped to 48565b1 (memory node kind + MemoryProvider trait, not modified here).
📝 WalkthroughWalkthroughAdds a first-class tinyflows ChangesWorkflow memory capability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
- memory_adapter.rs: fully-qualify serde_json::Value::as_array inside a tracing::debug! field expression — tracing's field-capture macro shadows a bare `Value` path segment with its own trait, so `Value::as_array` failed to resolve (E0782). - flows/tools.rs: add the missing NodeKind::Memory arm to config_hint's match (E0004, non-exhaustive patterns) — renders "operation · scope" (e.g. "recall · flow") for the flow summary's config_hint field.
… through the engine
Proves the memory node through the FULL stack: tinyflows compile/run
dispatching through the real OpenHumanMemory host adapter (build_capabilities),
against a real on-disk Memory store — not the crate's MockMemory, and not the
adapter tested in isolation.
- flow-scope remember -> recall round-trip across two separate compiled
graphs/engine runs under the same TrustedAutomation{Workflow} flow_id
- coherence: the memory-node write is visible via the sibling
flow_memory_recall agent tool (tinyhumansai#5176) against the same store
- security invariant end-to-end: a remember/scope:"user" node is rejected at
tinyflows' own validate/compile step, the adapter independently hard-refuses
it too, and the user's real GLOBAL_NAMESPACE store stays untouched
- dry_run_workflow's MockMemory path still works with a memory node and never
touches the real store
No bugs found in the adapter/engine wiring itself.
…ebc36ac47 — merge main + companion caps fix)
…074aa8f08 — review fixes)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5afd8114c1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "Canonical dedupe pattern (see the worked example in the memory-node design doc): \ | ||
| split_out → memory[recall·flow, query=\"=item.id\"] → \ | ||
| condition(\"=nodes.<id>.item.json.found\") → act on the false branch → \ | ||
| memory[remember·flow, key=\"=item.id\"] AFTER the action. Remembering after — \ |
There was a problem hiding this comment.
When the builder follows this new canonical pattern, the condition reads item.json.found, but the host adapter shapes recall/search results as { scope, query, results: [...] } and the updated prompt examples elsewhere use results[0] instead. For generated dedupe flows this makes the branch condition resolve to null/false even when the memory lookup found a prior item, so already-processed items can be reprocessed rather than skipped.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .with_note( | ||
| "Canonical dedupe pattern (see the worked example in the memory-node design doc): \ | ||
| split_out → memory[recall·flow, query=\"=item.id\"] → \ | ||
| condition(\"=nodes.<id>.item.json.found\") → act on the false branch → \ | ||
| memory[remember·flow, key=\"=item.id\"] AFTER the action. Remembering after — \ | ||
| not before — the action means a failed action never falsely marks an item as \ | ||
| done.", | ||
| ), |
There was a problem hiding this comment.
found field
The canonical dedupe pattern note instructs builders to write condition("=nodes.<id>.item.json.found"), but shape_recall_result in memory_adapter.rs returns { "scope", "query", "results": [...] } — there is no found field. A condition node binding item.json.found will always evaluate to null/falsy, meaning the "not found" branch fires on every run and the dedup never actually suppresses duplicates. This is the primary advertised use case for the memory node, so a silently broken condition here is a high-impact documentation defect.
prompt.md correctly uses results[0].id for the same pattern, and the e2e tests in memory_node_e2e_tests.rs only assert on results (never found), confirming found is not in the output shape. The found field does appear in the flavour operation's output (line 317 of memory_adapter.rs), which may be the source of confusion.
| let action = json!({ "operation": "remember", "scope": scope, "key": key }); | ||
| self.tier_gate_write("remember", &action).await?; | ||
|
|
||
| let content = value_to_content(&value); | ||
| if crate::openhuman::memory_store::safety::has_likely_secret(&content) { | ||
| tracing::warn!( | ||
| target: "flows", | ||
| key_chars = key.chars().count(), | ||
| content_chars = content.chars().count(), | ||
| "{LOG_PREFIX} remember: REFUSED — content looks like a secret" | ||
| ); | ||
| return Err(EngineError::Capability( | ||
| "memory node: refusing to store content that looks like a secret".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
tier_gate_write (line 401) — which calls intercept_audited via gate_call_for_tier and parks for human approval on flows where require_approval: true — runs before the has_likely_secret check on line 404. If the stored value triggers the heuristic, the user has already approved an action that will silently fail: the HITL interaction is wasted, and the commit-on-success pattern the PR itself advocates leaves the flow in an inconsistent state.
The fix is to move the value_to_content + has_likely_secret block above the tier_gate_write call, so validation always precedes side-effects that require user interaction.
| /// Backs both `recall` and `search` (`opts.operation` distinguishes them | ||
| /// purely for the returned envelope's `operation` field — both currently | ||
| /// route through the same [`Memory::recall`] call; there is no separate | ||
| /// hybrid-search path reachable through the generic `Arc<dyn Memory>` | ||
| /// trait object this adapter holds). |
There was a problem hiding this comment.
operation in the output envelope is incorrect
The comment says opts.operation is used "purely for the returned envelope's operation field", but shape_recall_result returns { "scope", "query", "results" } — no operation field. The extracted operation variable is only used in tracing::debug! calls.
| pub struct OpenHumanMemory { | ||
| pub config: Arc<Config>, | ||
| pub security: Arc<SecurityPolicy>, | ||
| } |
There was a problem hiding this comment.
pub fields on a security-sensitive struct
Both config and security are pub. Since the struct is only constructed in caps::build_capabilities and in test helpers, pub(crate) would be strictly correct and would prevent accidental out-of-crate construction with a relaxed SecurityPolicy.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
| Filename | Overview |
|---|---|
| src/openhuman/tinyflows/memory_adapter.rs | New adapter implementing MemoryProvider for the memory node; two previously-flagged ordering issues remain (secret check after HITL gate, pub fields on security struct) |
| src/openhuman/flows/agents/workflow_builder/prompt.md | Builder prompt now teaches the memory node; both canonical dedupe examples use results[0].id (memory entry UUID) instead of results[0].key, producing a condition that always evaluates false |
| src/openhuman/flows/memory_tools.rs | cross_flow_recall extracted and exported; FlowMemoryRecallTool now delegates to the shared function; no issues found |
| src/openhuman/flows/node_contracts.rs | Memory node overlay added; the dedupe note still references .found (previously flagged); overlay test and count updated correctly to 13 |
| src/openhuman/tinyflows/memory_adapter_tests.rs | Good unit tests covering scope lockdown, tier gate, trusted-origin enforcement, and flavour/people delegation; no issues found |
| src/openhuman/tinyflows/memory_node_e2e_tests.rs | E2E integration tests for all six memory operations and coherence between node and agent-tool paths; assertions correctly use results array, never .found |
| src/openhuman/memory/tools/flavour.rs | lookup_flavour extracted as pub(crate) function; MemoryFlavourTool now delegates to it; clean refactor with no behavioral change |
| src/openhuman/tinyflows/caps.rs | enforce_node_tier_gate and gate_call_for_tier promoted to pub(crate) for memory adapter reuse; OpenHumanMemory wired into build_capabilities; no issues found |
| src/openhuman/flows/tools.rs | Memory node added to ProposeWorkflowTool description and config_hint; inline dedupe snippet still uses .found (same issue already flagged in node_contracts.rs) |
Sequence Diagram
sequenceDiagram
participant Engine as tinyflows Engine
participant Adapter as OpenHumanMemory
participant Gate as enforce_node_tier_gate
participant HITL as gate_call_for_tier
participant Store as Memory Store
Note over Engine,Store: Read path
Engine->>Adapter: recall(scope, query, opts)
Adapter->>Gate: CommandClass::Read
Gate-->>Adapter: Allow / Block
alt "scope=user"
Adapter->>Store: recall(GLOBAL_NAMESPACE)
else "scope=flow"
Adapter->>Adapter: trusted_flow_id()
Adapter->>Store: recall(flow_id)
else "scope=flows"
Adapter->>Store: "cross_flow_recall(flow_* only)"
end
Store-->>Adapter: Vec MemoryEntry
Adapter-->>Engine: shape_recall_result
Note over Engine,Store: Write path
Engine->>Adapter: "remember(scope=flow, key, value)"
Adapter->>Adapter: "hard-refuse scope != flow"
Adapter->>Gate: CommandClass::Write
Gate-->>Adapter: tier decision
Adapter->>HITL: gate_call_for_tier
HITL-->>Adapter: Allow / Deny
Adapter->>Adapter: has_likely_secret check
Adapter->>Adapter: trusted_flow_id()
Adapter->>Store: store_with_taint(flow_id, key, ExternalSync)
Reviews (2): Last reviewed commit: "chore(deps): update Tauri shell Cargo.lo..." | Re-trigger Greptile
| ``` | ||
| trigger → tool_call (fetch candidates) → split_out (one item per candidate) | ||
| → memory [recall · flow, query="=item.id"] | ||
| → condition ("=nodes.<mem_id>.item.json.results[0].id == item.id") |
There was a problem hiding this comment.
results[0].id is the memory store's internal entry UUID (generated by the store, unrelated to what was passed to remember.key). Comparing it to item.id will always return false, so the "found" branch never fires and every item is re-processed every run — the dedup never suppresses anything.
Per shape_recall_result in memory_adapter.rs, the field that holds the stored key name is results[0].key. The correct condition is either results[0].key == item.id (exact stored-key match, since you stored with key="=item.id") or (.nodes.<mem_id>.item.json.results | length) > 0 (any recall hit). The same wrong expression appears a second time at line 609 in the "The memory node" canonical pattern block.
- P1: remove the broken recall→condition dedupe recipe from the builder prompt + node_contracts overlay (semantic recall can't do exact keyed membership; 'found' field never existed, results[0].id is a store UUID). Kept general memory-node teaching; noted exact dedup is deferred to a dedicated primitive. Guard test now asserts the recipe is absent. - P1: run the has_likely_secret rejection BEFORE tier_gate_write/HITL so a likely-secret value is refused up front, not after wasting an approval. - P2: pub -> pub(crate) on the adapter's config/security fields. - P2: correct the stale opts.operation doc comment. - P2: register the memory node in about_app catalog. - fix invalid 'email-tone' flavour slug in the prompt -> valid persona facet.
Review comments addressed — pushed in
|
|
To use Codex here, create a Codex account and connect to github. |
coderabbitai
Bot
added
feature
![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
🧹 Nitpick comments (4)
src/openhuman/tinyflows/memory_adapter.rs (3)
509-519: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueASCII-only case folding misses non-ASCII name matches.
to_ascii_lowercaseleaves accented/non-Latin characters untouched, so"JOSÉ"won't match a stored"José"while"JOSE"-style queries work.to_lowercase()on both sides fixes the common cases at negligible cost for a ≤100-entry list.🤖 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/tinyflows/memory_adapter.rs` around lines 509 - 519, Update the query normalization and field matching closure around needle and matches to use Unicode-aware to_lowercase() for both the query and candidate fields instead of to_ascii_lowercase(), preserving the existing contains and missing-field behavior.
420-423: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBoth write paths gate for approval before verifying the trusted flow origin.
tier_gate_writecan park the run on a human approval round-trip, and only afterwards doesflow_memory_namespace()enforce theTrustedAutomation { Workflow }origin — so a write that can never succeed still consumes an approval prompt. Hoisting the namespace resolution above the gate applies the same fail-fast ordering already adopted for the likely-secret check.
src/openhuman/tinyflows/memory_adapter.rs#L420-L423: resolveself.flow_memory_namespace()?beforeself.tier_gate_write("remember", &action).src/openhuman/tinyflows/memory_adapter.rs#L469-L472: resolveself.flow_memory_namespace()?beforeself.tier_gate_write("forget", &action).🤖 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/tinyflows/memory_adapter.rs` around lines 420 - 423, In src/openhuman/tinyflows/memory_adapter.rs lines 420-423, update the remember write path to resolve flow_memory_namespace() before calling tier_gate_write("remember", &action), while preserving the existing namespace use. Apply the same ordering in src/openhuman/tinyflows/memory_adapter.rs lines 469-472 for the forget path, resolving flow_memory_namespace() before tier_gate_write("forget", &action).
142-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApproved memory writes never record their execution outcome in the audit trail.
gate_call_for_tierreturns anaudit_id, which is dropped here.OpenHumanHttp::requestuses it to callgate.record_execution(&id, Success/Failure, …)after dispatch, so an approved-then-failed write is visible in the audit log. As written, aremember/forgetthat passes approval and then fails atstore_with_taint/forgetleaves an approval record with no outcome.Threading the id back to the call sites (or having
tier_gate_writereturn it) would keep memory writes consistent with the other acting adapters.🤖 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/tinyflows/memory_adapter.rs` around lines 142 - 154, The tier_gate_write method currently discards the audit_id returned by gate_call_for_tier, so approved memory writes cannot record execution outcomes. Return the audit ID from tier_gate_write and update the remember/forget call sites to record Success or Failure via the gate execution-audit API after store_with_taint or forget completes, preserving denial handling.src/openhuman/flows/memory_tools.rs (1)
129-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
tracing(target"flows") for the new helper's diagnostics, and add an entry/exit line.Everywhere else in this file (
execute, and the adapter that now calls this helper) emitstracing::debug!/warn!withtarget: "flows"; thelog::warn!here won't carry that target and won't be picked up by the same filters flow debugging relies on. This helper is also now a shared entry point for both the tool and thememorynode, so an entry/exit line with namespace/hit counts is worth having.♻️ Suggested diagnostics alignment
) -> anyhow::Result<Vec<MemoryEntry>> { let summaries = memory.namespace_summaries().await?; + tracing::debug!( + target: "flows", + namespace_count = summaries.len(), + limit, + ?min_score, + "[flows:memory] cross_flow_recall: entry" + ); let mut merged: Vec<MemoryEntry> = Vec::new(); @@ Err(e) => { - log::warn!( - "[flows:memory] cross_flow_recall failed for namespace={}: {e}", - summary.namespace + tracing::warn!( + target: "flows", + namespace = %summary.namespace, + error = %e, + "[flows:memory] cross_flow_recall: per-namespace recall failed — skipping" ); } @@ merged.truncate(limit); + tracing::debug!( + target: "flows", + hit_count = merged.len(), + "[flows:memory] cross_flow_recall: merged" + ); Ok(merged) }As per coding guidelines: "New or changed flows must include verbose, grep-friendly diagnostics for entry/exit, branches, external calls, retries/timeouts, 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/flows/memory_tools.rs` around lines 129 - 164, Update cross_flow_recall to use tracing diagnostics with target "flows" instead of log::warn!, and add grep-friendly entry and exit debug lines that include the query/limit context plus the namespaces processed and total hits returned. Preserve the existing per-namespace warning behavior while aligning all new logs with the tracing target.Source: Coding guidelines
🤖 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 `@src/openhuman/flows/tools.rs`:
- Around line 72-80: Update the memory tool description in the flow
documentation string to state that config.scope is required for search as well
as recall, remember, and forget. Keep the existing scope access rules unchanged
and ensure the description matches the validation contract documented in
prompt.md.
In `@src/openhuman/tinyflows/memory_adapter.rs`:
- Around line 357-365: Update the people lookup flow around handle_list and
filter_people_by_query so query-based requests search beyond the
DEFAULT_PEOPLE_LIMIT window, using a sufficiently large or query-aware limit
before filtering. Preserve the current 100-entry limit for requests without a
query and keep the existing result shaping behavior.
- Around line 201-206: Clamp the parsed limit in the options handling before it
is passed to the store, ensuring values are bounded to the allowed range of 1
through the defined maximum. Update the limit computation near min_score and
reuse an existing MAX constant if available, while preserving the default of 5
and the current u64-to-usize conversion.
---
Nitpick comments:
In `@src/openhuman/flows/memory_tools.rs`:
- Around line 129-164: Update cross_flow_recall to use tracing diagnostics with
target "flows" instead of log::warn!, and add grep-friendly entry and exit debug
lines that include the query/limit context plus the namespaces processed and
total hits returned. Preserve the existing per-namespace warning behavior while
aligning all new logs with the tracing target.
In `@src/openhuman/tinyflows/memory_adapter.rs`:
- Around line 509-519: Update the query normalization and field matching closure
around needle and matches to use Unicode-aware to_lowercase() for both the query
and candidate fields instead of to_ascii_lowercase(), preserving the existing
contains and missing-field behavior.
- Around line 420-423: In src/openhuman/tinyflows/memory_adapter.rs lines
420-423, update the remember write path to resolve flow_memory_namespace()
before calling tier_gate_write("remember", &action), while preserving the
existing namespace use. Apply the same ordering in
src/openhuman/tinyflows/memory_adapter.rs lines 469-472 for the forget path,
resolving flow_memory_namespace() before tier_gate_write("forget", &action).
- Around line 142-154: The tier_gate_write method currently discards the
audit_id returned by gate_call_for_tier, so approved memory writes cannot record
execution outcomes. Return the audit ID from tier_gate_write and update the
remember/forget call sites to record Success or Failure via the gate
execution-audit API after store_with_taint or forget completes, preserving
denial handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3bedecf-90a1-4884-a2ec-426a51db2122
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
src/openhuman/about_app/catalog_data.rssrc/openhuman/flows/agents/workflow_builder/builder_prompt.rssrc/openhuman/flows/agents/workflow_builder/prompt.mdsrc/openhuman/flows/builder_tools.rssrc/openhuman/flows/builder_tools_tests.rssrc/openhuman/flows/memory_tools.rssrc/openhuman/flows/mod.rssrc/openhuman/flows/node_contracts.rssrc/openhuman/flows/tools.rssrc/openhuman/memory/tools.rssrc/openhuman/memory/tools/flavour.rssrc/openhuman/tinyflows/caps.rssrc/openhuman/tinyflows/memory_adapter.rssrc/openhuman/tinyflows/memory_adapter_tests.rssrc/openhuman/tinyflows/memory_node_e2e_tests.rssrc/openhuman/tinyflows/mod.rssrc/openhuman/tools/ops.rsvendor/tinyflows
| config required), sub_workflow (config.workflow: an embedded child WorkflowGraph), \ | ||
| memory (config.operation REQUIRED: recall | search | flavour | people | remember | \ | ||
| forget; config.scope for recall/remember/forget: \"user\" is READ-ONLY, \"flow\" is \ | ||
| this flow's own memory and the ONLY scope remember/forget may target, \"flows\" is \ | ||
| cross-flow READ-ONLY; config.query for recall/search; config.flavour for the flavour \ | ||
| slug; config.key/config.value for remember/forget. Place remember AFTER the real \ | ||
| action it records, never before, so a failed action never marks an item as done. \ | ||
| Exact \"process each item once\" dedup is not reliably expressible via semantic \ | ||
| recall — don't improvise a recall/condition dedupe graph). If \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Document scope as required for search too.
Line 74 omits search, while prompt.md states that scope is required for recall/search/remember/forget. Builders following this description can generate memory-search nodes that fail validation.
Proposed fix
- forget; config.scope for recall/remember/forget: "user" is READ-ONLY, \
+ forget; config.scope for recall/search/remember/forget: "user" is READ-ONLY, \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config required), sub_workflow (config.workflow: an embedded child WorkflowGraph), \ | |
| memory (config.operation REQUIRED: recall | search | flavour | people | remember | \ | |
| forget; config.scope for recall/remember/forget: \"user\" is READ-ONLY, \"flow\" is \ | |
| this flow's own memory and the ONLY scope remember/forget may target, \"flows\" is \ | |
| cross-flow READ-ONLY; config.query for recall/search; config.flavour for the flavour \ | |
| slug; config.key/config.value for remember/forget. Place remember AFTER the real \ | |
| action it records, never before, so a failed action never marks an item as done. \ | |
| Exact \"process each item once\" dedup is not reliably expressible via semantic \ | |
| recall — don't improvise a recall/condition dedupe graph). If \ | |
| config required), sub_workflow (config.workflow: an embedded child WorkflowGraph), \ | |
| memory (config.operation REQUIRED: recall | search | flavour | people | remember | \ | |
| forget; config.scope for recall/search/remember/forget: \"user\" is READ-ONLY, \"flow\" is \ | |
| this flow's own memory and the ONLY scope remember/forget may target, \"flows\" is \ | |
| cross-flow READ-ONLY; config.query for recall/search; config.flavour for the flavour \ | |
| slug; config.key/config.value for remember/forget. Place remember AFTER the real \ | |
| action it records, never before, so a failed action never marks an item as done. \ | |
| Exact \"process each item once\" dedup is not reliably expressible via semantic \ | |
| recall — don't improvise a recall/condition dedupe graph). If \ |
🤖 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/flows/tools.rs` around lines 72 - 80, Update the memory tool
description in the flow documentation string to state that config.scope is
required for search as well as recall, remember, and forget. Keep the existing
scope access rules unchanged and ensure the description matches the validation
contract documented in prompt.md.
| #[allow(clippy::cast_possible_truncation)] | ||
| let limit = opts | ||
| .get("limit") | ||
| .and_then(Value::as_u64) | ||
| .map_or(5, |v| v as usize); | ||
| let min_score = opts.get("min_score").and_then(Value::as_f64); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clamp limit before it reaches the store.
opts.limit is taken verbatim from node config (author/model-supplied) with no upper bound. For scope: "flows" it is applied per namespace inside cross_flow_recall and all results are accumulated into one Vec before truncation, so a large value multiplies across every flow_* namespace. A cheap .clamp(1, MAX) here keeps a mis-authored node from ballooning a run's memory.
🛡️ Suggested clamp
+ const MAX_RECALL_LIMIT: usize = 100;
#[allow(clippy::cast_possible_truncation)]
let limit = opts
.get("limit")
.and_then(Value::as_u64)
- .map_or(5, |v| v as usize);
+ .map_or(5, |v| (v as usize).clamp(1, MAX_RECALL_LIMIT));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[allow(clippy::cast_possible_truncation)] | |
| let limit = opts | |
| .get("limit") | |
| .and_then(Value::as_u64) | |
| .map_or(5, |v| v as usize); | |
| let min_score = opts.get("min_score").and_then(Value::as_f64); | |
| const MAX_RECALL_LIMIT: usize = 100; | |
| #[allow(clippy::cast_possible_truncation)] | |
| let limit = opts | |
| .get("limit") | |
| .and_then(Value::as_u64) | |
| .map_or(5, |v| (v as usize).clamp(1, MAX_RECALL_LIMIT)); | |
| let min_score = opts.get("min_score").and_then(Value::as_f64); |
🤖 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/tinyflows/memory_adapter.rs` around lines 201 - 206, Clamp the
parsed limit in the options handling before it is passed to the store, ensuring
values are bounded to the allowed range of 1 through the defined maximum. Update
the limit computation near min_score and reuse an existing MAX constant if
available, while preserving the default of 5 and the current u64-to-usize
conversion.
| const DEFAULT_PEOPLE_LIMIT: usize = 100; | ||
| let outcome = crate::openhuman::people::rpc::handle_list(&store, DEFAULT_PEOPLE_LIMIT) | ||
| .await | ||
| .map_err(EngineError::Capability)?; | ||
|
|
||
| let shaped = match query { | ||
| None => outcome.value, | ||
| Some(q) => filter_people_by_query(outcome.value, q), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
query filters only the first 100 ranked people, so a match outside that window returns nothing.
handle_list is capped at DEFAULT_PEOPLE_LIMIT before filter_people_by_query runs, which makes a targeted lookup ("find Dana") silently return an empty list once the directory exceeds 100 entries — indistinguishable from "no such person" to the downstream node. Requesting a larger page when a query is present (or pushing the filter into the store query) would remove the false negative.
🛡️ Minimal mitigation
const DEFAULT_PEOPLE_LIMIT: usize = 100;
- let outcome = crate::openhuman::people::rpc::handle_list(&store, DEFAULT_PEOPLE_LIMIT)
+ // A filtered lookup must scan more than one ranked page, or a match
+ // ranked outside `DEFAULT_PEOPLE_LIMIT` looks like "no such person".
+ const FILTERED_PEOPLE_LIMIT: usize = 1_000;
+ let list_limit = if query.is_some() {
+ FILTERED_PEOPLE_LIMIT
+ } else {
+ DEFAULT_PEOPLE_LIMIT
+ };
+ let outcome = crate::openhuman::people::rpc::handle_list(&store, list_limit)
.await
.map_err(EngineError::Capability)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const DEFAULT_PEOPLE_LIMIT: usize = 100; | |
| let outcome = crate::openhuman::people::rpc::handle_list(&store, DEFAULT_PEOPLE_LIMIT) | |
| .await | |
| .map_err(EngineError::Capability)?; | |
| let shaped = match query { | |
| None => outcome.value, | |
| Some(q) => filter_people_by_query(outcome.value, q), | |
| }; | |
| const DEFAULT_PEOPLE_LIMIT: usize = 100; | |
| // A filtered lookup must scan more than one ranked page, or a match | |
| // ranked outside `DEFAULT_PEOPLE_LIMIT` looks like "no such person". | |
| const FILTERED_PEOPLE_LIMIT: usize = 1_000; | |
| let list_limit = if query.is_some() { | |
| FILTERED_PEOPLE_LIMIT | |
| } else { | |
| DEFAULT_PEOPLE_LIMIT | |
| }; | |
| let outcome = crate::openhuman::people::rpc::handle_list(&store, list_limit) | |
| .await | |
| .map_err(EngineError::Capability)?; | |
| let shaped = match query { | |
| None => outcome.value, | |
| Some(q) => filter_people_by_query(outcome.value, q), | |
| }; |
🤖 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/tinyflows/memory_adapter.rs` around lines 357 - 365, Update the
people lookup flow around handle_list and filter_people_by_query so query-based
requests search beyond the DEFAULT_PEOPLE_LIMIT window, using a sufficiently
large or query-aware limit before filtering. Preserve the current 100-entry
limit for requests without a query and keep the existing result shaping
behavior.
2 participants
Part of the memory-in-workflows track (#5150 #5175 #5176 #5205). Closes #5226 (host half). Depends on tinyhumansai/tinyflows#23 (the node kind +
MemoryProvidertrait) — the submodule gitlink is bumped to that branch; must land + gitlink re-pointed to the squash-merge SHA before this goes green/ready.What
The
OpenHumanMemoryadapter that makes the new tinyflowsmemorynode actually work in a running OpenHuman flow. Gives every flow declarative, in-graph memory: read (recall/search/flavour/people) atuser/flow/flowsscope, and durable flow-scoped write (remember/forget). This is the deterministic complement to the read-onlyflow_memory_agent(#5205) — and it's what lets a flow express "never repost" (dedup) asrecall·flow → condition → remember·flow,rememberafter the action = commit-on-success.Adapter delegation (
src/openhuman/tinyflows/memory_adapter.rs)user→Memory::recallonGLOBAL_NAMESPACE;flow→Memory::recallonflows::flow_namespace(flow_id);flows→ extractedflows::cross_flow_recall(shared withFlowMemoryRecallToolso both surfaces return identical results). Reads pass throughwrap_untrusted_for_agent.memory::tools::flavour::lookup_flavour(shared withMemoryFlavourTool).people::rpc::handle_list(same op aspeople_list) + substring filter.scope != "flow"(defense-in-depth beyond the engine's validate-time rejection), thenMemory::store_with_taint(flow_namespace(flow_id), …, ExternalSync)/Memory::forget.Security
flow_idis taken from the trustedTrustedAutomation{Workflow}turn origin — never a caller/graph-supplied value — so a flow can only ever touch its own namespace.flowscope uses the sameflow_<id>namespace as the feat(flows): per-flow memory namespace + post-run digest (flow:<id>) #5176 tools, the digest, and the memory agent — one consistent flow-memory store across all surfaces.enforce_node_tier_gate/gate_call_for_tier):Readfor recall/search/flavour/people,Writefor remember/forget (always routed through the HITL gate, so arequire_approvalflow parks). No new permission path.flowsCargo feature (leaf-gate).Also
node_contracts.rs(overlay + test),flows/tools.rs,flows/builder_tools.rs(+tests),tools/ops.rs.user; false forflow). Guard tests updated.48565b1.Follow-up (not in this PR)
graph_has_outbound_side_effectdoesn't forcerequire_approvalfor amemory[remember·flow]node the way it does fortool_call/http/code. Flow-scope writes are contained (own namespace, ExternalSync-tainted, tier-gate still parks underrequire_approval), so this is a deliberate low-risk gap — filing a separate issue.Testing
No
cargo build/cargo testlocally (per workflow — CI validates);rustfmt --checkclean. Adapter unit tests: user-scope read shape, flow-scope write→read coherence via the #5176 tool path, user/flows-scope write hard-error, tier-gate consultation, 13-kind overlay.Summary by CodeRabbit
New Features
Security
Bug Fixes