feat(nodes): add memory node kind (recall/flavour/people + flow-scoped remember) - #23
Conversation
…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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
| Filename | Overview |
|---|---|
| src/nodes/integration/memory.rs | New memory node executor; handles all six operations with correct per-item/once dispatch, defense-in-depth scope guard for write ops, and comprehensive tests. |
| src/validate.rs | Memory validation block correctly rejects both scope:"user" and scope:"flows" for write ops, enforces per-operation required fields, and is covered by 14+ new tests including the flows-write rejection. |
| src/caps/mod.rs | Adds MemoryProvider trait (5 methods, async_trait) and optional memory field to Capabilities; mirrors the AgentRunner Optional pattern exactly. |
| src/caps/mock.rs | Adds shaped MockMemory (not a naive echo), wires it into mock_capabilities by default, and provides mock_capabilities_with_memory for custom test overrides. |
| src/catalog.rs | Adds memory contract with six operations, scope enum, and a HARD SECURITY RULE note; note names only scope:"user" as the hard reject without explicitly calling out scope:"flows", though "writes may only target scope "flow"" is present. |
| src/model/node_kind.rs | Adds Memory variant to NodeKind enum with doc comment referencing MemoryProvider and the write invariant. |
| src/nodes/mod.rs | Wires NodeKind::Memory to MemoryNode in executor_for dispatch; adds Memory to all_kinds test helper. |
| src/main.rs | Adds memory: None to standalone CLI capabilities; correct since the CLI has no memory store. |
Sequence Diagram
sequenceDiagram
participant G as WorkflowGraph
participant V as validate_all
participant E as MemoryNode executor
participant P as MemoryProvider (host)
G->>V: graph with memory node
V->>V: check operation enum
V->>V: "check scope enum (user|flow|flows)"
alt remember/forget + scope user or flows
V-->>G: ValidationError (hard reject)
else valid config
V-->>G: Ok(())
end
G->>E: NodeContext (config, input items)
E->>E: execution_mode (per_item / once)
loop per item (or once)
E->>E: "resolve_config (expand =expr)"
E->>E: check caps.memory is Some
alt "operation = recall | search"
E->>P: recall(scope, query, opts)
P-->>E: Value (results)
else "operation = flavour"
E->>P: flavour(slug)
P-->>E: Value (traits)
else "operation = people"
E->>P: people(query?)
P-->>E: Value (people list)
else "operation = remember"
E->>E: "guard scope == flow"
E->>P: remember(scope, key, value)
P-->>E: Ok(())
else "operation = forget"
E->>E: "guard scope == flow"
E->>P: forget(scope, key)
P-->>E: Ok(())
end
E->>E: envelope::wrap(result)
end
E-->>G: NodeOutput (items)
Reviews (4): Last reviewed commit: "fix(memory): reject flow-write to read-o..." | 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
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ect-drive writes - validate: remember/forget now reject scope "flows" (read-only), not just "user" — closes the write-restriction bypass (Greptile P1). Documents that the literal-enum scope check is what makes the invariant unbypassable (CodeRabbit). - executor: remember/forget hard-error unless scope=="flow", so a direct drive (bypassing validate) can never silently write an empty/read-only scope (CodeRabbit). Adds validate + executor regression tests.
Review comments addressed — pushed in
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Current commit and reported commit =='
git rev-parse --short HEAD
git show --no-patch --oneline b1f62f8 2>&1 || true
printf '%s\n' '== Relevant file outlines =='
ast-grep outline src/validate.rs --items all --type function 2>&1 || true
ast-grep outline src/nodes/integration/memory.rs --items all --type function,impl 2>&1 || true
printf '%s\n' '== Scope-validation and direct-write guard contexts =='
rg -n -C 12 -P 'remember|forget|scope|HARD SECURITY|read-only|write' \
src/validate.rs src/nodes/integration/memory.rs
printf '%s\n' '== Reported regression tests =='
rg -n -C 8 -P 'memory_rejects_remember_flows_scope|memory_rejects_forget_flows_scope|write_to_non_flow_scope_is_a_capability_error_even_when_driven_directly' \
src/validate.rs src/nodes/integration/memory.rs
printf '%s\n' '== Formatting check availability =='
if command -v rustfmt >/dev/null 2>&1; then
rustfmt --check src/validate.rs src/nodes/integration/memory.rs
else
echo 'rustfmt unavailable in sandbox'
fiLength of output: 67156
The One minor documentation follow-up remains: I could not independently run ✏️ Learnings added
|
|
All four fixes verify correctly against the code. validate.rs — the invariant now covers both read-only scopes: if matches!(operation, "remember" | "forget")
&& matches!(scope, Some("user") | Some("flows"))And the block comment explicitly documents that the literal-enum scope check is what makes the invariant unbypassable — including the warning that making memory.rs — both write arms hard-error on anything that isn't if scope != "flow" {
return Err(EngineError::Capability(...));
}This covers the empty-scope default case ( Test coverage —
Everything looks good. LGTM. Tip: You can customize Greptile's behavior for this repo with |
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
recall,search,flavour,people,remember, andforget, withper_itemandonceexecution modes.remember/forgetfor disallowedscopevalues.