feat(nodes): add memory node kind (recall/flavour/people + flow-scoped remember) - #23
feat(nodes): add memory node kind (recall/flavour/people + flow-scoped remember)#23graycyrus wants to merge 1 commit into
memory node kind (recall/flavour/people + flow-scoped remember)#23Conversation
…d remember) Adds the 13th node kind, `memory`, as a delivery surface onto a host-injected `MemoryProvider` capability (recall/search/flavour/people reads, remember/forget writes). Enforces the hard security invariant at validate time: a remember/forget operation may never target scope "user" — writes are restricted to flow-scoped memory, so a workflow can never plant or erase durable facts about the user. Includes a shaped MockMemory (wired into mock_capabilities by default) so dry-run keeps working, and node/validate/ catalog test coverage.
📝 WalkthroughWalkthroughAdds a host-backed ChangesMemory node capability and contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowEngine
participant MemoryNode
participant MemoryProvider
WorkflowEngine->>MemoryNode: Execute memory configuration
MemoryNode->>MemoryProvider: Invoke selected operation
MemoryProvider-->>MemoryNode: Return result or status
MemoryNode-->>WorkflowEngine: Return integration output
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
| Filename | Overview |
|---|---|
| src/nodes/integration/memory.rs | New executor for the memory node kind; well-structured per-item / once execution modes, per-operation dispatch, and solid in-file test coverage for all six operations. |
| src/validate.rs | Adds memory-node validation including the security invariant for remember/forget writes, but the guard only rejects scope: "user" — scope: "flows" (also documented as read-only) is accepted for write operations, inconsistent with the stated invariant and the error message text. |
| src/caps/mod.rs | Adds MemoryProvider trait mirroring the existing AgentRunner precedent; five well-documented methods; Capabilities::memory added as Option<Arc<dyn MemoryProvider>>. |
| src/caps/mock.rs | Adds MockMemory with shaped return values for all five trait methods; wired into mock_capabilities() by default; adds mock_capabilities_with_memory helper and tests. |
| src/catalog.rs | Adds the memory node's catalog contract with all config fields, scope enum, example, and the hard-security-rule note; NODE_KINDS bumped to 13. |
| src/model/node_kind.rs | Adds NodeKind::Memory variant with correct serde wire value "memory" and doc comment; wire-format round-trip test added. |
| src/nodes/mod.rs | Dispatches NodeKind::Memory to MemoryNode; all_kinds() test helper updated to include Memory. |
Comments Outside Diff (1)
-
src/validate.rs, line 1221-1230 (link)scope: "flows"write ops not blocked at validate timeThe hard-invariant check only rejects
scope: "user"forremember/forget, but the"flows"scope is documented throughout this PR as "cross-flow read access — read-only". Both the validator error message ("remember/forget may only write scope "flow"") and the catalog note ("writes may only target scope "flow"") make it clear that"flows"should also be blocked. A workflow graph containing{ "operation": "remember", "scope": "flows", "key": "k", "value": 1 }passesvalidate_alltoday and will callMemoryProvider::rememberwithscope = "flows", relying on the host to reject what the validator promised it would never see. There is also no test covering this path.
Reviews (1): Last reviewed commit: "feat(nodes): add memory node kind (recal..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/caps/mod.rs (1)
249-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdding a public field to
Capabilitiesbreaks hosts that build it with a struct literal.
Capabilitieshas no#[non_exhaustive], so every host constructing it directly must now addmemory: None. Worth calling out in release notes (or adding a constructor/Default-style builder) so the compile break is expected.🤖 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/caps/mod.rs` around lines 249 - 254, Update the public Capabilities API to avoid an unannounced struct-literal compatibility break from the memory field: add #[non_exhaustive] to Capabilities or provide and adopt a constructor/Default-style builder that centralizes the new field, and document the required migration in release notes if applicable. Preserve the optional memory behavior exposed by the memory field.src/validate.rs (1)
179-200: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winWorth a comment that the scope enum check is what makes the invariant unbypassable.
The user-scope ban only holds because line 191 rejects any non-literal
scope(an"=expr"binding fails the enum check), so a runtime-resolved scope can never reachprovider.remember/forget. A future change allowing bindablescopewould silently reopen that hole — worth stating explicitly next to the invariant.🤖 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/validate.rs` around lines 179 - 200, The validation around the scope check in validate.rs should explicitly document that restricting scope to the literal enum values prevents bindable or runtime-resolved scopes from bypassing the user-scope prohibition for remember/forget operations. Add a concise comment beside the invariant or enum validation, referencing the relevant validation logic without changing behavior.
🤖 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/nodes/integration/memory.rs`:
- Line 70: Update scope handling in the integration executor for recall,
remember, and forget so write operations do not default a missing or non-string
scope to an empty string. Require a valid scope for these operations and return
the documented Capability error when it is absent or invalid, while preserving
the existing operation-specific behavior.
- Around line 124-143: Run cargo fmt --all to apply rustfmt formatting
throughout the affected memory integration code, including the remember/forget
EngineError constructors, cfg.get(...).and_then(...).ok_or_else(...) chain,
opts.insert call, and assert! statement. Do not change behavior.
---
Nitpick comments:
In `@src/caps/mod.rs`:
- Around line 249-254: Update the public Capabilities API to avoid an
unannounced struct-literal compatibility break from the memory field: add
#[non_exhaustive] to Capabilities or provide and adopt a
constructor/Default-style builder that centralizes the new field, and document
the required migration in release notes if applicable. Preserve the optional
memory behavior exposed by the memory field.
In `@src/validate.rs`:
- Around line 179-200: The validation around the scope check in validate.rs
should explicitly document that restricting scope to the literal enum values
prevents bindable or runtime-resolved scopes from bypassing the user-scope
prohibition for remember/forget operations. Add a concise comment beside the
invariant or enum validation, referencing the relevant validation logic without
changing behavior.
🪄 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: 4c8e31cc-0dc4-44d2-a3ba-3f9f82eb4f89
📒 Files selected for processing (9)
README.mdsrc/caps/mock.rssrc/caps/mod.rssrc/catalog.rssrc/model/node_kind.rssrc/nodes/integration/memory.rssrc/nodes/integration/mod.rssrc/nodes/mod.rssrc/validate.rs
| let operation = cfg.get("operation").and_then(Value::as_str).ok_or_else(|| { | ||
| EngineError::Capability("memory node: missing `operation` in config".to_string()) | ||
| })?; | ||
| let scope = cfg.get("scope").and_then(Value::as_str).unwrap_or(""); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Write operations silently fall back to an empty scope.
scope defaults to "" when absent or non-string, and that empty string is passed straight to remember/forget. The validator requires scope for those ops, so this only bites when the executor is driven directly — but the module doc (Lines 22-27) claims this executor defends against malformed config with a Capability error, and a write to an unspecified scope is worse than a hard error. Consider requiring scope for recall/remember/forget here rather than defaulting.
Also applies to: 137-137, 149-149
🤖 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/nodes/integration/memory.rs` at line 70, Update scope handling in the
integration executor for recall, remember, and forget so write operations do not
default a missing or non-string scope to an empty string. Require a valid scope
for these operations and return the documented Capability error when it is
absent or invalid, while preserving the existing operation-specific behavior.
| let key = cfg.get("key").and_then(Value::as_str).ok_or_else(|| { | ||
| EngineError::Capability("memory node: `remember` operation requires `key`".to_string()) | ||
| })?; | ||
| let value = cfg.get("value").cloned().ok_or_else(|| { | ||
| EngineError::Capability( | ||
| "memory node: `remember` operation requires `value`".to_string(), | ||
| ) | ||
| })?; | ||
| tracing::debug!( | ||
| node = %ctx.node.id, | ||
| key, | ||
| "{LOG_PREFIX} calling provider.remember" | ||
| ); | ||
| provider.remember(scope, key, value).await?; | ||
| serde_json::json!({ "ok": true, "operation": "remember", "key": key }) | ||
| } | ||
| "forget" => { | ||
| let key = cfg.get("key").and_then(Value::as_str).ok_or_else(|| { | ||
| EngineError::Capability("memory node: `forget` operation requires `key`".to_string()) | ||
| })?; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
cargo fmt --check fails on this file — CI is red.
Both CI jobs report formatting diffs (the EngineError::Capability(...) one-liners here at Lines 125 and 142 exceed the width rustfmt would wrap at, plus the cfg.get(...).and_then(...).ok_or_else(...) chain, the opts.insert call, and the assert! around Line 294). Run cargo fmt --all.
🔧 Proposed formatting fix for the two flagged constructors
let key = cfg.get("key").and_then(Value::as_str).ok_or_else(|| {
- EngineError::Capability("memory node: `remember` operation requires `key`".to_string())
+ EngineError::Capability(
+ "memory node: `remember` operation requires `key`".to_string(),
+ )
})?; let key = cfg.get("key").and_then(Value::as_str).ok_or_else(|| {
- EngineError::Capability("memory node: `forget` operation requires `key`".to_string())
+ EngineError::Capability(
+ "memory node: `forget` operation requires `key`".to_string(),
+ )
})?;📝 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.
| let key = cfg.get("key").and_then(Value::as_str).ok_or_else(|| { | |
| EngineError::Capability("memory node: `remember` operation requires `key`".to_string()) | |
| })?; | |
| let value = cfg.get("value").cloned().ok_or_else(|| { | |
| EngineError::Capability( | |
| "memory node: `remember` operation requires `value`".to_string(), | |
| ) | |
| })?; | |
| tracing::debug!( | |
| node = %ctx.node.id, | |
| key, | |
| "{LOG_PREFIX} calling provider.remember" | |
| ); | |
| provider.remember(scope, key, value).await?; | |
| serde_json::json!({ "ok": true, "operation": "remember", "key": key }) | |
| } | |
| "forget" => { | |
| let key = cfg.get("key").and_then(Value::as_str).ok_or_else(|| { | |
| EngineError::Capability("memory node: `forget` operation requires `key`".to_string()) | |
| })?; | |
| let key = cfg.get("key").and_then(Value::as_str).ok_or_else(|| { | |
| EngineError::Capability( | |
| "memory node: `remember` operation requires `key`".to_string(), | |
| ) | |
| })?; | |
| let value = cfg.get("value").cloned().ok_or_else(|| { | |
| EngineError::Capability( | |
| "memory node: `remember` operation requires `value`".to_string(), | |
| ) | |
| })?; | |
| tracing::debug!( | |
| node = %ctx.node.id, | |
| key, | |
| "{LOG_PREFIX} calling provider.remember" | |
| ); | |
| provider.remember(scope, key, value).await?; | |
| serde_json::json!({ "ok": true, "operation": "remember", "key": key }) | |
| } | |
| "forget" => { | |
| let key = cfg.get("key").and_then(Value::as_str).ok_or_else(|| { | |
| EngineError::Capability( | |
| "memory node: `forget` operation requires `key`".to_string(), | |
| ) | |
| })?; |
🧰 Tools
🪛 GitHub Actions: CI / 1_Rust SDK.txt
[error] 139-139: cargo fmt --check failed (formatting diff detected). Run cargo fmt --all to apply the suggested formatting.
🪛 GitHub Actions: CI / Rust SDK
[error] 139-139: cargo fmt --all -- --check failed due to formatting differences (EngineError Capability string/constructor multiline formatting). Run cargo fmt --all to apply rustfmt.
🤖 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/nodes/integration/memory.rs` around lines 124 - 143, Run cargo fmt --all
to apply rustfmt formatting throughout the affected memory integration code,
including the remember/forget EngineError constructors,
cfg.get(...).and_then(...).ok_or_else(...) chain, opts.insert call, and assert!
statement. Do not change behavior.
Source: Pipeline failures
1 participant
What
Adds the 13th node kind,
memory, as a delivery surface onto a new host-injectedMemoryProvidercapability. Lets a running workflow read memory (recall/search/flavour/people) and durably write flow-scoped memory (remember/forget) declaratively, in-graph.Consumed by OpenHuman (tinyhumansai/openhuman#5226) — the host supplies the real
MemoryProvider; this crate stays host-agnostic.Design
NodeKind::Memory, wire value"memory". Additive —CURRENT_SCHEMA_VERSIONstays1.MemoryProvidertrait +Capabilities::memory: Option<Arc<dyn MemoryProvider>>— mirrors the existingagent: Option<Arc<dyn AgentRunner>>slot exactly (hosts without memory keep compiling; amemorynode without a provider fails withEngineError::Capability, never a silent no-op).nodes/integration/memory.rs, default execution modePerItem(sosplit_out → memoryfans out one call per item).Config
operationscopequery=-bindableflavourkey/value=-bindablelimit,min_scoreoperation: searchmaps torecall()withopts.operation = "search"so a host can distinguish semantic recall from full-text search; a host that doesn't care ignores it.Hard security invariant (validate-time)
operation: remember|forget+scope: "user"is rejected invalidate_all— structurally, where the builder sees it, not mid-run. A workflow can never plant or erase durable facts about the user; writes are restricted to flow scope. Plus required-field + closed-enum-scope checks per the table.Dry-run
MockMemory(shaped returns) wired intomock_capabilities()by default, sodry_run_workflowkeeps working.Tests
Node executor (recall/flavour/people/remember/forget against
MockMemory),=-expression resolution, the validate-time user-write rejection (+ remember·flow passes), required-field errors, and the catalog contract (NODE_KINDS = 13,memorycontract present).Note
#![forbid(unsafe_code)]/#![warn(missing_docs)]honored — every new public item documented.Summary by CodeRabbit
New Features
Documentation