Repository navigation
feat(memory): AMA-Agent causality-graph MemoryService provider - #6694
Open
pranavraja99 wants to merge 5 commits into
Open
pranavraja99 wants to merge 5 commits into
pranavraja99 wants to merge 5 commits into
Conversation
A Rust port of the memory system from "AMA-Bench: Evaluating Long-Horizon Memory for Agentic Applications" (arXiv:2602.22769) and its reference implementation, plugged into the third-party provider lane PR #6345 opened. Third arm of a memory-backend comparison (ironclaw native vs mem0 vs this), measured on the same suites with the same model. The reference impl is Python with no service mode, so a faithful in-ironclaw comparison needs a real native provider rather than a subprocess wrapper. Modules (48 unit tests, clippy-clean): - graph.rs pure, zero-I/O causality graph + every retrieval primitive - embedding.rs crate-local trait; real OpenAI-compatible client + offline stub - url_check.rs SSRF gate adapted from ironclaw_memory_mem0 - store.rs per-scope JSON persistence over ironclaw_filesystem - llm.rs extraction + sufficiency-judgment prompts and parsing - service.rs the MemoryService impl - config.rs knobs defaulted to upstream's configs/ama_agent.yaml Deliberate divergences, all documented in the crate docs rather than hidden: * BOTH NEED_GRAPH strategies ship, selectable via `graph_retrieval_mode`. The paper describes walking the causality graph; the reference implementation's retrieve.py never consults causal_graph at all and instead returns turns by index. They disagree, so both are implemented to MEASURE the difference instead of assuming one. * NEED_AGGREGATE replaces NEED_CODE. Upstream generates Python and executes it in a subprocess inheriting the parent environment. The same class of count/list/pattern queries is answered by a fixed native menu, so a memory provider adds no arbitrary-code-execution surface. Bounded fidelity loss against a paper-reported ~23.5% of queries. * Construction is incremental (ironclaw delivers turns via record_interaction) rather than upstream's one batch pass over a finished trajectory. * Prompts are adapted to strict JSON, not transcribed, matching this codebase's typed-contract convention; the loose-substring fallback upstream relies on is retained as a second chance. Behavior worth noting: - Unsupported ops (write/read/tree/profile_*) are deliberately NOT overridden, inheriting the trait's fail-closed `unavailable` default instead of returning something plausible but wrong. Recorded in the mapping-fidelity table. - Extraction and the sufficiency judgment both DEGRADE rather than fail: memory must never break a turn. Bad extraction still records the raw turn; a bad judgment falls back to plain similarity retrieval. - Edges whose endpoints were not extracted are dropped, so a hallucinated endpoint cannot create a dangling node id that traversal follows into nothing. - search() filters zero-similarity hits. top_k returns the k NEAREST regardless of distance, which suits context retrieval but makes a model-facing search tool emit noise the model may then reason from. - Corrupt stored graph is an error, not a silent reset — a silent reset is indistinguishable from a genuine recall failure in a benchmark.
Wires the causality-graph provider into the same compose-time selection path mem0 uses, so `[memory].provider = "ama-agent.local.memory"` now resolves to a real MemoryService instead of failing closed as an unknown id. - assets/memory_ama_agent/manifest.toml: identity-only manifest (declares no tools of its own). Declares only context_retrieval + interaction_log, NOT document_store — a causality graph has no addressable-document model, matching the crate's mapping-fidelity table. - memory_native_extension.rs: extension-id / service / manifest constants. - memory_provider_factory.rs: AmaAgentConnectionConfig + a feature-gated arm and create_ama_agent_provider(). Fails closed with a SPECIFIC logged reason for each missing prerequisite (graph filesystem, embedding endpoint/model, chat endpoint/model) rather than one generic failure — a provider that silently retrieves nothing is indistinguishable from genuinely poor recall in a benchmark. An unrecognized graph_retrieval_mode is likewise rejected, since quietly defaulting would publish a mislabelled comparison arm. - input.rs: `with_ama_agent_memory_connection` builder, mirroring the mem0 pair. - factory.rs: build_ama_agent_graph_filesystem() mounts a dedicated local dir at /memory over `<local-dev root>/ama-agent-memory`. Why a dedicated mount rather than the composed root filesystem: third-party providers are constructed ONCE at startup (line ~3247), but the composed root filesystem is not assembled until much later (~3625) — which is exactly why `for_third_party` passes `filesystem: None`. mem0 doesn't notice (it is pure HTTP); this provider needs durable storage, so it gets its own narrow mount instead of reordering the build. - chat.rs (new, in the provider crate): a small OpenAI-compatible LlmProvider. The composed model gateway lives in ironclaw_operator (layer `products`), which a `substrates` crate may not depend on, and the provider is built synchronously before any async provider factory runs. So it owns its own client, sharing the embedder's SSRF gate. Documented that ironclaw_llm's retry/cost instrumentation is deliberately not reused: both calls are best-effort and already degrade. - ironclaw_reborn_config: `[memory.ama_agent]` section (endpoints, top_k, graph_retrieval_mode, graph_depth), secrets via env only. - ARCHITECTURE TEST GENERALIZED: reborn_dependency_boundaries' provider-neutrality test hardcoded "ironclaw_memory_mem0", so it would have silently failed to enforce the rule for this new crate. Now table-driven over every concrete provider crate. - Updated docs/plans/composition-pubuse.snapshot for the intentional facade addition (AmaAgentConnectionConfig). Verified: cargo check with the feature on; 51 crate tests; all ironclaw_architecture tests green (incl. the generalized boundary test + the manifest ratchet); 33 host_runtime memory tests; clippy clean on both crates.
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
scope: docs
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6694
July 27, 2026 05:02
Destroyed
github-actions
Bot
added
size: XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an AMA-Agent causality-graph memory provider with OpenAI-compatible clients, graph extraction and retrieval, scoped persistence, configuration, composition wiring, extension registration, and dependency-boundary coverage. ChangesAMA-Agent memory provider
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RebornHostBindings
participant MemoryProviderFactory
participant AmaAgentMemoryService
participant AmaLlm
participant GraphStore
RebornHostBindings->>MemoryProviderFactory: provide AMA-Agent connection
MemoryProviderFactory->>AmaAgentMemoryService: construct provider with filesystem and clients
AmaAgentMemoryService->>AmaLlm: extract interaction or judge evidence
AmaAgentMemoryService->>GraphStore: load or update scoped graph
GraphStore-->>AmaAgentMemoryService: graph state
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
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 |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6694
July 27, 2026 05:02
Destroyed
|
🚅 Deployed to the ironclaw-pr-6694 environment in ironclaw-ci-preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/input.rs (1)
380-398: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMisplaced doc comment:
with_ama_agent_memory_connectioninherited the wrong doc block.The pre-existing doc (Lines 380-384, "Attach a resolved memory profile binding policy... reads the resolved policy via the
memory_binding_policyfield") describeswith_memory_binding_policy, not the newly insertedwith_ama_agent_memory_connection. Since Rust merges contiguous///lines into one doc comment,with_memory_binding_policy(Line 395) is now left completely undocumented, and rustdoc forwith_ama_agent_memory_connectionmisleadingly opens with a description of a different field.📝 Proposed fix
- /// Attach a resolved memory profile binding policy (issue `#3537`). The CLI - /// resolves this from the `[memory]` config section + deployment profile, - /// failing closed before composition is built. The factory reads the - /// resolved policy via the `memory_binding_policy` field when destructuring - /// the build input. /// Attach AMA-Agent memory settings. Only consulted when the binding policy /// selects that provider; inert otherwise. pub fn with_ama_agent_memory_connection( mut self, connection: crate::AmaAgentConnectionConfig, ) -> Self { self.ama_agent_memory_connection = Some(connection); self } + /// Attach a resolved memory profile binding policy (issue `#3537`). The CLI + /// resolves this from the `[memory]` config section + deployment profile, + /// failing closed before composition is built. The factory reads the + /// resolved policy via the `memory_binding_policy` field when destructuring + /// the build input. pub fn with_memory_binding_policy(mut self, policy: MemoryBindingPolicy) -> Self { self.memory_binding_policy = Some(policy); self }🤖 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 `@crates/ironclaw_reborn_composition/src/input.rs` around lines 380 - 398, Move the resolved memory profile binding policy documentation from `with_ama_agent_memory_connection` to `with_memory_binding_policy`, keeping the AMA-Agent memory settings documentation attached only to `with_ama_agent_memory_connection`. Ensure both methods have accurate, separate rustdoc comments.
🤖 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 `@crates/ironclaw_memory_ama_agent/src/graph.rs`:
- Around line 52-59: Update the persisted enums NodeKind and GraphEdge to add
serde’s snake_case rename convention alongside their existing Serialize and
Deserialize derives. Apply #[serde(rename_all = "snake_case")] to both enum
declarations so graph.json round-tripping uses stable snake_case wire names.
- Around line 79-84: Update upsert_edge to normalize Association endpoints into
a deterministic order before checking for or inserting the edge, so reversed a/b
inputs compare as the same logical association. Leave Causal endpoint direction
unchanged and apply the same normalization at the other upsert_edge location.
In `@crates/ironclaw_memory_ama_agent/src/store.rs`:
- Around line 95-110: Replace the process-local write_lock read-modify-write in
AmaAgentStore::update (crates/ironclaw_memory_ama_agent/src/store.rs:95-110)
with the shared bounded cas_update helper, preserving mutation and serialization
error handling while avoiding a mutex across I/O. In record_interaction
(crates/ironclaw_memory_ama_agent/src/service.rs:196-233), move
processed_turn_run_ids.contains and max turn_idx + 1 computation into the
store.update mutate closure so both use the freshest graph atomically; propagate
graph-load errors instead of converting Err(_) to 0, unless an inline silent-ok
justification documents the fallback.
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 792-813: Replace the silent `VirtualPath::new("/memory").ok()?` in
the filesystem setup with an explicit error branch that logs the
path-construction failure via `tracing::warn!` using the existing memory target
and then returns `None`, matching the `create_dir_all` and `mount_local`
branches in this function.
- Around line 3283-3301: Extend the ama_agent_filesystem match in the factory
wiring to also handle RebornStorageInput::HostedSingleTenantPostgres when
ama_agent_memory_connection is present, using its root with
build_ama_agent_graph_filesystem just like LocalDev. Preserve None for
unsupported storage profiles, and ensure any intentionally unsupported profile
is identified in the resulting warning rather than failing with a generic
message.
In `@crates/ironclaw_reborn_composition/src/memory_provider_factory.rs`:
- Around line 296-409: Add composition-layer tests in the existing #[cfg(test)]
module for create_ama_agent_provider, covering fail-closed results for missing
graph filesystem, missing embedding endpoint/model, missing chat endpoint/model,
and unknown graph_retrieval_mode, plus a valid configuration that constructs a
provider. Reuse the module’s existing dependency/configuration setup and assert
each invalid path returns None while the valid path returns Some.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/input.rs`:
- Around line 380-398: Move the resolved memory profile binding policy
documentation from `with_ama_agent_memory_connection` to
`with_memory_binding_policy`, keeping the AMA-Agent memory settings
documentation attached only to `with_ama_agent_memory_connection`. Ensure both
methods have accurate, separate rustdoc comments.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad0f2d6b-06d0-4802-bc69-d6784baef98e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (22)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_host_runtime/assets/memory_ama_agent/manifest.tomlcrates/ironclaw_host_runtime/src/memory_native_extension.rscrates/ironclaw_memory_ama_agent/Cargo.tomlcrates/ironclaw_memory_ama_agent/src/chat.rscrates/ironclaw_memory_ama_agent/src/config.rscrates/ironclaw_memory_ama_agent/src/embedding.rscrates/ironclaw_memory_ama_agent/src/error.rscrates/ironclaw_memory_ama_agent/src/graph.rscrates/ironclaw_memory_ama_agent/src/lib.rscrates/ironclaw_memory_ama_agent/src/llm.rscrates/ironclaw_memory_ama_agent/src/service.rscrates/ironclaw_memory_ama_agent/src/store.rscrates/ironclaw_memory_ama_agent/src/url_check.rscrates/ironclaw_reborn_composition/Cargo.tomlcrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/memory_provider_factory.rscrates/ironclaw_reborn_config/src/config_file.rsdocs/plans/composition-pubuse.snapshot
Comment on lines
+52
to
+59
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub enum NodeKind { | ||
| /// Environment state — what the world looks like (objects, positions, files, | ||
| /// query results, error text). | ||
| EnvState, | ||
| /// Objective/task state — progress toward the goal, sub-goals, plans. | ||
| TaskState, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Persisted enums missing #[serde(rename_all = "snake_case")].
NodeKind and GraphEdge are round-tripped through graph.json (see store.rs's serde_json::to_vec/from_slice) but default-derive PascalCase wire names instead of an explicit convention.
🔧 Proposed fix
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "snake_case")]
pub enum NodeKind {
EnvState,
TaskState,
}
...
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "snake_case")]
pub enum GraphEdge {
Causal { from: NodeId, to: NodeId },
Association { a: NodeId, b: NodeId },
}Also applies to: 78-84
🤖 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 `@crates/ironclaw_memory_ama_agent/src/graph.rs` around lines 52 - 59, Update
the persisted enums NodeKind and GraphEdge to add serde’s snake_case rename
convention alongside their existing Serialize and Deserialize derives. Apply
#[serde(rename_all = "snake_case")] to both enum declarations so graph.json
round-tripping uses stable snake_case wire names.
Source: Coding guidelines
Comment on lines
+79
to
+84
| pub enum GraphEdge { | ||
| /// `from` caused / was a precondition of `to`. | ||
| Causal { from: NodeId, to: NodeId }, | ||
| /// `a` and `b` co-occur / are associated, no direction implied. | ||
| Association { a: NodeId, b: NodeId }, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
upsert_edge doesn't normalize Association endpoint order, so the "no direction implied" invariant can be violated by duplicate edges.
Two extractions of the same fact in reversed textual order produce Association{a,b} and Association{a:b,b:a}, which Vec::contains treats as distinct — silently duplicating a logically identical edge.
🔧 Proposed fix
pub fn upsert_edge(&mut self, edge: GraphEdge) {
+ let edge = match edge {
+ GraphEdge::Association { a, b } if a > b => GraphEdge::Association { a: b, b: a },
+ other => other,
+ };
if !self.edges.contains(&edge) {
self.edges.push(edge);
}
}Also applies to: 199-204
🤖 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 `@crates/ironclaw_memory_ama_agent/src/graph.rs` around lines 79 - 84, Update
upsert_edge to normalize Association endpoints into a deterministic order before
checking for or inserting the edge, so reversed a/b inputs compare as the same
logical association. Leave Causal endpoint direction unchanged and apply the
same normalization at the other upsert_edge location.
Comment on lines
+95
to
+110
| pub async fn update<F>(&self, scope: &ResourceScope, mutate: F) -> Result<(), AmaAgentError> | ||
| where | ||
| F: FnOnce(&mut CausalityGraph) -> Result<(), AmaAgentError> + Send, | ||
| { | ||
| let _guard = self.write_lock.lock().await; | ||
| let mut graph = self.load(scope).await?; | ||
| mutate(&mut graph)?; | ||
| let bytes = serde_json::to_vec(&graph) | ||
| .map_err(|e| AmaAgentError::Storage(format!("serialize graph: {e}")))?; | ||
| let path = Self::graph_path(scope)?; | ||
| self.filesystem | ||
| .write_file(&path, &bytes) | ||
| .await | ||
| .map_err(|e| AmaAgentError::Storage(format!("write graph.json: {e}")))?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Read-modify-write isn't CAS-backed, and callers compute derived state from stale unlocked reads — turns can be silently lost under concurrency.
The persistence layer holds a process-local mutex across filesystem I/O instead of using a versioned/CAS update, and its only caller derives next_idx and the idempotency verdict from separate unlocked reads taken before that lock — so two concurrent record_interaction calls on the same scope can compute the same next_idx and the second silently overwrites the first's turn via upsert_turn.
crates/ironclaw_memory_ama_agent/src/store.rs#L95-L110: Replace thewrite_lock: Mutex<()>read-then-write pattern with the repo's shared bounded CAS helper (cas_update) so writes are versioned against the filesystem's own state, not a process-local lock that only covers one instance.crates/ironclaw_memory_ama_agent/src/service.rs#L196-L233: Move the idempotency check (processed_turn_run_ids.contains) and thenext_idxcomputation (max turn_idx + 1) inside themutateclosure passed tostore.update, where they see the freshest graph under the same atomic section as the write — don't pre-read the graph twice outside the lock. Also replaceErr(_) => 0with propagating the load error (?) or an explicit// silent-ok:-justified fallback, per the fail-loud guideline.
As per coding guidelines: "Every read-modify-write must use the shared bounded CAS update path; do not overwrite a previously read version without CAS," "Do not hold a process-local or per-record async mutex across filesystem or backend I/O," and "do not use ... warning-only error handling that continues with a potentially uninitialized value for DB, I/O, workspace, or settings reads... justified fallbacks must include an inline // silent-ok: <reason> comment."
📍 Affects 2 files
crates/ironclaw_memory_ama_agent/src/store.rs#L95-L110(this comment)crates/ironclaw_memory_ama_agent/src/service.rs#L196-L233
🤖 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 `@crates/ironclaw_memory_ama_agent/src/store.rs` around lines 95 - 110, Replace
the process-local write_lock read-modify-write in AmaAgentStore::update
(crates/ironclaw_memory_ama_agent/src/store.rs:95-110) with the shared bounded
cas_update helper, preserving mutation and serialization error handling while
avoiding a mutex across I/O. In record_interaction
(crates/ironclaw_memory_ama_agent/src/service.rs:196-233), move
processed_turn_run_ids.contains and max turn_idx + 1 computation into the
store.update mutate closure so both use the freshest graph atomically; propagate
graph-load errors instead of converting Err(_) to 0, unless an inline silent-ok
justification documents the fallback.
Source: Coding guidelines
Comment on lines
+792
to
+813
| let host_root = root.join("ama-agent-memory"); | ||
| if let Err(error) = std::fs::create_dir_all(&host_root) { | ||
| tracing::warn!( | ||
| target: "ironclaw_reborn::memory", | ||
| %error, | ||
| "could not create the ama-agent graph directory; failing the binding closed" | ||
| ); | ||
| return None; | ||
| } | ||
| let virtual_root = VirtualPath::new("/memory").ok()?; | ||
| let host_path = HostPath::from_path_buf(host_root); | ||
| let mut filesystem = DiskFilesystem::new(); | ||
| if let Err(error) = filesystem.mount_local(virtual_root, host_path) { | ||
| tracing::warn!( | ||
| target: "ironclaw_reborn::memory", | ||
| %error, | ||
| "could not mount the ama-agent graph directory; failing the binding closed" | ||
| ); | ||
| return None; | ||
| } | ||
| Some(Arc::new(filesystem) as Arc<dyn RootFilesystem>) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Silent .ok()? breaks the function's own fail-loud pattern.
VirtualPath::new("/memory").ok()? (Line 801) swallows the error and returns None without logging, unlike the create_dir_all and mount_local failure branches immediately around it, which both call tracing::warn! with a specific reason before failing closed. If this ever fails (e.g., a future change to VirtualPath validation), an operator gets no diagnostic at all for why ama-agent memory silently stopped working.
🔧 Proposed fix
- let virtual_root = VirtualPath::new("/memory").ok()?;
+ let virtual_root = match VirtualPath::new("/memory") {
+ Ok(path) => path,
+ Err(error) => {
+ tracing::warn!(
+ target: "ironclaw_reborn::memory",
+ %error,
+ "could not construct the ama-agent graph virtual mount path; failing the binding closed"
+ );
+ return None;
+ }
+ };As per coding guidelines: "do not use ... .ok()? ... that continues with a potentially uninitialized value for DB, I/O, workspace, or settings reads. Fail loudly with ? by default; justified fallbacks must include an inline // silent-ok: <reason> comment naming the operation."
📝 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.
Suggested change
| let host_root = root.join("ama-agent-memory"); | |
| if let Err(error) = std::fs::create_dir_all(&host_root) { | |
| tracing::warn!( | |
| target: "ironclaw_reborn::memory", | |
| %error, | |
| "could not create the ama-agent graph directory; failing the binding closed" | |
| ); | |
| return None; | |
| } | |
| let virtual_root = VirtualPath::new("/memory").ok()?; | |
| let host_path = HostPath::from_path_buf(host_root); | |
| let mut filesystem = DiskFilesystem::new(); | |
| if let Err(error) = filesystem.mount_local(virtual_root, host_path) { | |
| tracing::warn!( | |
| target: "ironclaw_reborn::memory", | |
| %error, | |
| "could not mount the ama-agent graph directory; failing the binding closed" | |
| ); | |
| return None; | |
| } | |
| Some(Arc::new(filesystem) as Arc<dyn RootFilesystem>) | |
| } | |
| let host_root = root.join("ama-agent-memory"); | |
| if let Err(error) = std::fs::create_dir_all(&host_root) { | |
| tracing::warn!( | |
| target: "ironclaw_reborn::memory", | |
| %error, | |
| "could not create the ama-agent graph directory; failing the binding closed" | |
| ); | |
| return None; | |
| } | |
| let virtual_root = match VirtualPath::new("/memory") { | |
| Ok(path) => path, | |
| Err(error) => { | |
| tracing::warn!( | |
| target: "ironclaw_reborn::memory", | |
| %error, | |
| "could not construct the ama-agent graph virtual mount path; failing the binding closed" | |
| ); | |
| return None; | |
| } | |
| }; | |
| let host_path = HostPath::from_path_buf(host_root); | |
| let mut filesystem = DiskFilesystem::new(); | |
| if let Err(error) = filesystem.mount_local(virtual_root, host_path) { | |
| tracing::warn!( | |
| target: "ironclaw_reborn::memory", | |
| %error, | |
| "could not mount the ama-agent graph directory; failing the binding closed" | |
| ); | |
| return None; | |
| } | |
| Some(Arc::new(filesystem) as Arc<dyn RootFilesystem>) | |
| } |
🤖 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 `@crates/ironclaw_reborn_composition/src/factory.rs` around lines 792 - 813,
Replace the silent `VirtualPath::new("/memory").ok()?` in the filesystem setup
with an explicit error branch that logs the path-construction failure via
`tracing::warn!` using the existing memory target and then returns `None`,
matching the `create_dir_all` and `mount_local` branches in this function.
Source: Coding guidelines
Comment on lines
+3283
to
+3301
| // The AMA-Agent provider persists its causality graph, but the composed | ||
| // root filesystem does not exist yet at this point in the build (it is | ||
| // assembled further down), and a third-party provider is constructed once | ||
| // here rather than per-invocation. So give it a small dedicated local mount | ||
| // under the local-dev root instead of waiting for the composed filesystem. | ||
| // Non-local-dev storage yields `None`, which fails the arm closed. | ||
| let ama_agent_filesystem: Option<Arc<dyn RootFilesystem>> = | ||
| match (&storage, &ama_agent_memory_connection) { | ||
| (crate::input::RebornStorageInput::LocalDev { root, .. }, Some(_)) => { | ||
| build_ama_agent_graph_filesystem(root) | ||
| } | ||
| _ => None, | ||
| }; | ||
| let deps = crate::MemoryProviderDeps::for_third_party(memory_provider_connection) | ||
| .with_ama_agent( | ||
| ama_agent_memory_connection.unwrap_or_default(), | ||
| ama_agent_filesystem, | ||
| ); | ||
| crate::build_memory_service_resolver(memory_binding_policy, &deps) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)factory\.rs$|(^|/)input\.rs$|CLAUDE\.md|AGENTS\.md|\.claude/rules' || true
echo
echo "== relevant factory snippet outline/lines =="
wc -l crates/ironclaw_reborn_composition/src/factory.rs crates/ironclaw_reborn_composition/src/input.rs
sed -n '3260,3315p' crates/ironclaw_reborn_composition/src/factory.rs
echo
echo "== RebornStorageInput definitions/usages =="
rg -n "enum RebornStorageInput|HostedSingleTenantPostgres|build_ama_agent_graph_filesystem|ama_agent_memory_connection|no graph filesystem|RootFilesystem" crates/ironclaw_reborn_composition/src/input.rs crates/ironclaw_reborn_composition/src/factory.rs -C 4
echo
echo "== AMa-agent graph filesystem implementation =="
rg -n "fn build_ama_agent_graph_filesystem|build_ama_agent_graph_filesystem|ama.agent|AMA-Agent|graph filesystem" crates -C 8Repository: nearai/ironclaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== build_ama_agent_graph_filesystem implementation =="
sed -n '760,815p' crates/ironclaw_reborn_composition/src/factory.rs
echo
echo "== memory resolver error/warning paths =="
rg -n "no graph filesystem was supplied|Graph filesystem|with_ama_agent|No graph filesystem|Graph filesystem|ama-agent|MemoryProviderDeps|build_memory_service_resolver" crates/ironclaw_memory crates/ironclaw_reborn_composition crates -g '*.rs' -C 4
echo
echo "== input.rs structs around RebornStorageInput/build_hosted_single_tenant_postgres =="
sed -n '280,315p' crates/ironclaw_reborn_composition/src/input.rs
sed -n '450,535p' crates/ironclaw_reborn_composition/src/input.rs
echo
echo "== deterministic shape check: enum variants with root field and match pattern =="
python3 - <<'PY'
from pathlib import Path
p = Path('crates/ironclaw_reborn_composition/src/input.rs')
text = p.read_text()
print('RebornStorageInput contains LocalDev:', 'LocalDev {' in text and 'root: PathBuf,' in text[text.find('enum RebornStorageInput'):text.find('pub(crate) type BuildInput')])
print('RebornStorageInput contains HostedSingleTenantPostgres:', 'HostedSingleTenantPostgres {' in text and text.find('HostedSingleTenantPostgres {') < p.read_text().find('pub(crate) type BuildInput'))
s = text[text.find('enum RebornStorageInput'):text.find('pub(crate) type BuildInput')]
for name in ['LocalDev', 'HostedSingleTenantPostgres']:
i = s.find(name)
print('LocalDev declares root before enum end:', 'LocalDev {' in s[s.find('LocalDev'):s.find('HostedSingleTenantPostgres')])
print('HostedSingleTenantPostgres declares root before enum end:', 'HostedSingleTenantPostgres {' in s[s.find('HostedSingleTenantPostgres'):s.find('}",', s.find('HostedSingleTenantPostgres'))+3])
PY
echo
echo "== exact factory match context =="
sed -n '3272,3305p' crates/ironclaw_reborn_composition/src/factory.rsRepository: nearai/ironclaw
Length of output: 50372
Allow AMA-Agent memory graph mounts on HostedSingleTenantPostgres.
RebornStorageInput::HostedSingleTenantPostgres also carries root: PathBuf, but the match arm only builds ama_agent_filesystem for LocalDev; with ama-agent.local.memory selected, the provider fails closed with the generic “no graph filesystem was supplied” warning. If this profile is supported and root is available, mirror the local-dev case in crates/ironclaw_reborn_composition/src/factory.rs. If the profile is intentionally excluded, name the restriction in the warning.
Violates CLAUDE.md/AGENTS.md: changed Reborn crates must expose production profile support without silently failing closed, and product/wiring errors should name unsupported/incorrect 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 `@crates/ironclaw_reborn_composition/src/factory.rs` around lines 3283 - 3301,
Extend the ama_agent_filesystem match in the factory wiring to also handle
RebornStorageInput::HostedSingleTenantPostgres when ama_agent_memory_connection
is present, using its root with build_ama_agent_graph_filesystem just like
LocalDev. Preserve None for unsupported storage profiles, and ensure any
intentionally unsupported profile is identified in the resulting warning rather
than failing with a generic message.
Comment on lines
+296
to
+409
| /// Build the AMA-Agent causality-graph provider, or `None` (fail-closed). | ||
| /// | ||
| /// Fails closed — with a specific reason logged — when any of the four things it | ||
| /// cannot work without is absent: a graph filesystem, an embedding endpoint, a | ||
| /// chat endpoint, or a model id for either. Silently returning a provider that | ||
| /// retrieves nothing would look exactly like genuinely poor recall in a | ||
| /// benchmark, which is the failure mode this must not have. | ||
| #[cfg(feature = "memory-ama-agent")] | ||
| fn create_ama_agent_provider(deps: &MemoryProviderDeps) -> Option<Arc<dyn MemoryService>> { | ||
| let cfg = &deps.ama_agent; | ||
|
|
||
| let Some(filesystem) = deps.ama_agent_filesystem.clone() else { | ||
| tracing::warn!( | ||
| target: LOG_TARGET, | ||
| "ama-agent memory binding selected but no graph filesystem was supplied; failing closed" | ||
| ); | ||
| return None; | ||
| }; | ||
| let (Some(embed_url), Some(embed_model)) = ( | ||
| cfg.embedding_base_url.as_deref(), | ||
| cfg.embedding_model.as_deref(), | ||
| ) else { | ||
| tracing::warn!( | ||
| target: LOG_TARGET, | ||
| "ama-agent memory binding selected but the embedding endpoint/model is unset ([memory.ama_agent].embedding_base_url / .embedding_model); failing closed" | ||
| ); | ||
| return None; | ||
| }; | ||
| let (Some(llm_url), Some(llm_model)) = (cfg.llm_base_url.as_deref(), cfg.llm_model.as_deref()) | ||
| else { | ||
| tracing::warn!( | ||
| target: LOG_TARGET, | ||
| "ama-agent memory binding selected but the chat endpoint/model is unset ([memory.ama_agent].llm_base_url / .llm_model); failing closed" | ||
| ); | ||
| return None; | ||
| }; | ||
|
|
||
| let embedder = match OpenAiCompatEmbedder::new( | ||
| embed_url, | ||
| cfg.embedding_api_key | ||
| .as_ref() | ||
| .map(|k| k.expose_secret().to_string()), | ||
| embed_model, | ||
| cfg.embedding_dimension.unwrap_or(1536), | ||
| ) { | ||
| Ok(e) => Arc::new(e) as Arc<dyn AmaEmbeddingProvider>, | ||
| Err(error) => { | ||
| tracing::warn!( | ||
| target: LOG_TARGET, | ||
| %error, | ||
| "failed to build the ama-agent embedding client (rejected base URL); failing closed" | ||
| ); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| let chat = match OpenAiCompatChat::new( | ||
| llm_url, | ||
| cfg.llm_api_key | ||
| .as_ref() | ||
| .map(|k| k.expose_secret().to_string()), | ||
| llm_model, | ||
| ) { | ||
| Ok(c) => Arc::new(c) as Arc<dyn ironclaw_llm::LlmProvider>, | ||
| Err(error) => { | ||
| tracing::warn!( | ||
| target: LOG_TARGET, | ||
| %error, | ||
| "failed to build the ama-agent chat client (rejected base URL); failing closed" | ||
| ); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| // An unrecognized mode is rejected rather than silently defaulted: the two | ||
| // modes are the thing being compared, so quietly running the wrong one would | ||
| // publish a mislabelled result. | ||
| let graph_retrieval_mode = match cfg.graph_retrieval_mode.as_deref() { | ||
| None => AmaAgentConfig::default().graph_retrieval_mode, | ||
| Some("edge_traversal") => GraphRetrievalMode::EdgeTraversal, | ||
| Some("turn_window") => GraphRetrievalMode::TurnWindow, | ||
| Some(other) => { | ||
| tracing::warn!( | ||
| target: LOG_TARGET, | ||
| mode = other, | ||
| "unknown [memory.ama_agent].graph_retrieval_mode (expected edge_traversal|turn_window); failing closed" | ||
| ); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| let defaults = AmaAgentConfig::default(); | ||
| let config = AmaAgentConfig { | ||
| top_k: cfg.top_k.unwrap_or(defaults.top_k), | ||
| graph_retrieval_mode, | ||
| graph_depth: cfg.graph_depth.unwrap_or(defaults.graph_depth), | ||
| ..defaults | ||
| }; | ||
|
|
||
| tracing::info!( | ||
| target: LOG_TARGET, | ||
| top_k = config.top_k, | ||
| graph_depth = config.graph_depth, | ||
| mode = ?config.graph_retrieval_mode, | ||
| "built the ama-agent causality-graph memory provider" | ||
| ); | ||
| Some(Arc::new(AmaAgentMemoryService::new( | ||
| GraphStore::new(filesystem), | ||
| embedder, | ||
| AmaLlm::new(chat), | ||
| config, | ||
| )) as Arc<dyn MemoryService>) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for composition-layer tests covering create_ama_agent_provider / build_ama_agent_graph_filesystem.
rg -n -C3 'create_ama_agent_provider|build_ama_agent_graph_filesystem' crates/ironclaw_reborn_compositionRepository: nearai/ironclaw
Length of output: 3328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'crates/ironclaw_reborn_composition/**' | sed -n '1,120p'
echo
echo "== test/module markers =="
rg -n '#\[cfg\(.*test\)\]|mod tests|#\[cfg_attr|memory_provider_factory|ama_agent|MemoryProviderDeps|MemoryProviderBuilder|new_reborn_runtime|assemble_runtime' crates/ironclaw_reborn_composition/src -g '*.rs' | sed -n '1,240p'
echo
echo "== memory_provider_factory outline/snippet =="
wc -l crates/ironclaw_reborn_composition/src/memory_provider_factory.rs
sed -n '1,480p' crates/ironclaw_reborn_composition/src/memory_provider_factory.rsRepository: nearai/ironclaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== memory_provider_factory tests =="
sed -n '444,536p' crates/ironclaw_reborn_composition/src/memory_provider_factory.rs
echo
echo "== factory ama-agent test region =="
sed -n '3207,3320p' crates/ironclaw_reborn_composition/src/factory.rs
sed -n '5700,5880p' crates/ironclaw_reborn_composition/src/factory/tests.rs | rg -n -C4 'ama_agent|memory_ama_agent|graph_retrieval|AMA_AGENT|memory-provider|provider'
echo
echo "== repo coverage rule references =="
sed -n '1,220p' crates/ironclaw_reborn_composition/AGENTS.md
sed -n '1,240p' crates/ironclaw_reborn_composition/CLAUDE.md
rg -n "New or changed production-wired behavior|caller-level test|fails closed|production-wired" crates/ironclaw_reborn_composition/AGENTS.md crates/ironclaw_reborn_composition/CLAUDE.mdRepository: nearai/ironclaw
Length of output: 9851
Add composition-layer tests for the AMA-Agent provider factory paths.
create_document_store_provider has cases for native/mem0/unknown third-party, but the AMA-Agent branches only return None; memory_provider_factory’s #[cfg(test)] module covers only mem0. Per the ironclaw_reborn_composition invariant—new production-wired behavior needs a caller-level test at the nearest seam—the fail-closed cases (missing filesystem/endpoint/model/unknown graph mode) and successful construction need tests.
🤖 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 `@crates/ironclaw_reborn_composition/src/memory_provider_factory.rs` around
lines 296 - 409, Add composition-layer tests in the existing #[cfg(test)] module
for create_ama_agent_provider, covering fail-closed results for missing graph
filesystem, missing embedding endpoint/model, missing chat endpoint/model, and
unknown graph_retrieval_mode, plus a valid configuration that constructs a
provider. Reuse the module’s existing dependency/configuration setup and assert
each invalid path returns None while the valid path returns Some.
Source: Path instructions
Adds `RebornRuntime::memory_document_store()`, mirroring the existing `session_thread_service()` / `thread_scope()` accessors (same test-support gate). Motivation: a memory benchmark needs to SEED memory before asking recall questions. Without this, the only way to populate memory is to replay each trajectory turn through the model so the after-turn writer fires — one LLM call per step, which is prohibitive for a 100+ turn trajectory and also changes what is being measured. This exposes the same `MemoryService` the prompt-context lane and after-turn writer already use, so a harness can seed via record_interaction directly. It returns whichever provider the [memory] binding selected, so a harness can compare native / mem0 / ama-agent without knowing which is bound. Production is byte-identical: the field and accessor are both behind `cfg(any(test, feature = "test-support"))`, and the clone that feeds it is taken under the same gate before the value moves into after_turn_memory_writer. Verified: cargo check both with and without test-support; ironclaw_architecture tests green; fmt clean.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6694
July 27, 2026 05:26
Destroyed
…eding Companion to memory_document_store(): returns the ResourceScope built from the same thread_scope + actor user id the runtime's own memory paths use. Hand-building a scope outside the runtime is the exact failure this prevents — a mismatched user/agent/project axis writes to a different document and retrieval then finds nothing, which is indistinguishable from a memory backend that simply forgot. thread_id/mission_id are None so seeded writes land in the durable long-term lane rather than per-thread scratch. Same test-support gate as the sibling accessors; production unchanged.
serrrfirat
reviewed
Jul 27, 2026
| //! gateway, the provider owns a small client of its own — the same shape as | ||
| //! [`crate::embedding::OpenAiCompatEmbedder`], sharing its SSRF gate. | ||
| //! | ||
| //! Deliberately NOT reused here: `ironclaw_llm`'s retry/backoff and cost |
Collaborator
There was a problem hiding this comment.
I'm not sure if this is the right way to do this. We are gonna create implementation drift if we dont use ironclaw_llm. I rather fix the problem there and use the crate
serrrfirat
reviewed
Jul 27, 2026
| //! | ||
| //! | IronClaw op | AMA-Agent mapping | fidelity | | ||
| //! |----------------------|------------------------------------------------------|----------| | ||
| //! | `record_interaction` | LLM extraction -> nodes/edges/turn merged into graph | primary | |
Collaborator
There was a problem hiding this comment.
hmm do we somehow have an abstraction in front of these? Who is the caller of this? They would need to know which memory is active and have match arms probably @BenKurrek how did we design the new memory structure wrt to that?
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
ironclaw_memory_ama_agent— a causality-graphMemoryServiceprovider — and wires it into the compose-time[memory]binding lane opened by #6345.[memory].provider = "ama-agent.local.memory"now resolves to a real provider instead of failing closed as an unknown id.It's a Rust port of the memory system from AMA-Bench: Evaluating Long-Horizon Memory for Agentic Applications (arXiv:2602.22769) and its reference implementation.
Why
To make ironclaw's memory subsystem measurable against alternatives. We now have three arms selectable by config — native, mem0 (#6345), and this — so they can be run on the same suites with the same model.
Early data motivating it, on nearai-bench's
reborn_memorysuite (152 scenarios, deepseek-v4-flash, all runs 0 errors):Swapping the backend moves the number by ~3.7 points, so the abstraction #6345 introduced is doing real work. This adds a third, architecturally different arm.
The reference implementation is Python with no service mode, so a faithful in-ironclaw comparison needs a real native provider rather than a subprocess wrapper.
Design
Two stages, per the paper:
Mapping fidelity
Following
ironclaw_memory_mem0's convention:record_interactionretrieve_contextsearchwrite/read/treeprofile_set/profile_readUnsupported ops are not overridden, so they inherit the trait's fail-closed
unavailabledefault rather than returning something plausible but wrong. The manifest likewise declares onlycontext_retrieval+interaction_log, notdocument_store.Deliberate divergences from upstream
All documented in the crate docs rather than hidden:
NEED_GRAPHstrategies ship, selectable viagraph_retrieval_mode. The paper describes walking the causality graph; the reference implementation'sretrieve.pynever consultscausal_graphat all and instead returns neighbouring turns by index. They disagree, so both are implemented to measure the difference instead of picking one and asserting it.NEED_AGGREGATEreplacesNEED_CODE. Upstream generates Python and executes it in a subprocess that inherits the parent environment. The same class of count/list/pattern queries is answered here by a fixed native menu, so a memory provider adds no arbitrary-code-execution surface. Bounded, documented fidelity loss against a paper-reported ~23.5% of queries.record_interaction.Two things reviewers should look at
1. The provider carries its own LLM + embedding clients. The composed model gateway lives in
ironclaw_operator(layerproducts), which asubstratescrate may not depend on, and third-party providers are constructed synchronously before any async provider factory runs. Sochat.rsis a small OpenAI-compatibleLlmProvidersharing the embedder's SSRF gate.ironclaw_llm's retry/cost instrumentation is deliberately not reused — both calls are best-effort and already degrade — and that's documented at the call site rather than left implicit.2. A dedicated graph mount, not the composed root filesystem. Third-party providers are built at
factory.rs~3247, but the composed root filesystem isn't assembled until ~3625 — which is exactly whyfor_third_partypassesfilesystem: None. mem0 doesn't notice (pure HTTP); this provider needs durable storage. Rather than reorder the build, it gets a narrow/memorymount over<local-dev root>/ama-agent-memory. Open to a different approach if reordering is preferable.Fail-closed behavior
Every missing prerequisite (graph filesystem, embedding endpoint/model, chat endpoint/model) logs its own specific reason rather than one generic failure, and an unrecognized
graph_retrieval_modeis rejected instead of defaulted.That precision is deliberate: a memory provider that silently retrieves nothing is indistinguishable from genuinely poor recall in a benchmark. That's the one failure mode that would quietly corrupt the comparison this crate exists to run.
Architecture test generalized (please note)
reborn_dependency_boundaries's provider-neutrality test hardcoded"ironclaw_memory_mem0", so it would have silently failed to enforce the rule for this new crate. It's now table-driven over every concrete provider crate, so the next provider is covered automatically.docs/plans/composition-pubuse.snapshotis updated for the intentional facade addition (AmaAgentConnectionConfig).Config
Secrets come from env only:
MEMORY_AMA_AGENT_EMBEDDING_API_KEY,MEMORY_AMA_AGENT_LLM_API_KEY.Off by default — compiled only under
--features memory-ama-agent, same as mem0.Verification
cargo clippyclean on both touched crates;cargo fmt --checkcleanironclaw_architecturetests green, including the generalized boundary test and the manifest ratchetironclaw_host_runtimememory tests greencargo checkwithmemory-ama-agentenabledTwo bugs my own tests caught, worth mentioning because both would have skewed results rather than crashed: hallucinated edge endpoints created dangling node ids that traversal followed into nothing, and
searchreturned zero-similarity nodes as if relevant.Status
Not yet run end-to-end on a full suite — that's next, on
reborn_memoryand AMA-Bench. Opening now for review of the wiring and the two design calls above.🤖 Generated with Claude Code