Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds host-defined memory contracts and native extension assets, moves snippet sanitization and profile reads to the host, wires fail-closed memory binding and provider resolution through Reborn startup, introduces a mem0-backed third-party provider, and rewrites dependency-boundary checks to allowlists. ChangesMemory rollout
Allowlist-based dependency boundaries
Advisory ignore
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MemoryCapabilityState
participant MemoryServiceResolver
participant NativeMemoryService
participant ThirdPartyMemoryService
MemoryCapabilityState->>MemoryServiceResolver: resolve_document_store(filesystem, prompt_write_safety_event_sink)
alt native binding
MemoryServiceResolver->>NativeMemoryService: construct provider
NativeMemoryService-->>MemoryCapabilityState: Arc<dyn MemoryService>
else third-party binding
MemoryServiceResolver->>ThirdPartyMemoryService: return registered provider
ThirdPartyMemoryService-->>MemoryCapabilityState: Arc<dyn MemoryService>
end
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the memory context and user profile loading architecture, shifting the responsibility of reference hashing, sanitization, untrusted-envelope wrapping, and model-visible budget enforcement from the native memory provider to the host runtime. It also introduces a provider-neutral profile_read method to the MemoryService trait and updates workspace dependency boundary tests to use allowlists. Feedback suggests optimizing the snippet sanitization logic by caching the untrusted envelope prefix length using OnceLock to avoid redundant allocations on every snippet.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
BenKurrek
marked this pull request as draft
June 25, 2026 02:58
BenKurrek
changed the title
github-actions
Bot
added
scope: docs
BenKurrek
marked this pull request as ready for review
June 25, 2026 13:41
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs (1)
1712-1727: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "forbidden deps" wording after the denylist→allowlist switch.
Line 1715 still reads "Adding any of the forbidden deps here…", but rules no longer carry a
forbiddenlist — anything not inallowedis now rejected. Reword to the allowlist semantics so the contract comment matches the enforcement.🤖 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_architecture/tests/reborn_dependency_boundaries.rs` around lines 1712 - 1727, The contract comment in the `BoundaryRule` for `ironclaw_reborn_cli` still describes the rule as a forbidden-deps list, but the enforcement now uses an allowlist. Update the wording near the `allowed` field so it clearly says anything not in `allowed` is rejected, while keeping the same intent about protecting `ironclaw_reborn_composition`-mediated access to internal Reborn types.
🤖 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_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json`:
- Around line 7-10: Tighten the document-read.input.v1.json schema so the path
field is restricted to scoped relative paths only, not just any non-empty
string. Update the schema around the path property to enforce a relative,
non-absolute form and reject traversal patterns like leading slashes or
parent-directory segments, keeping the change localized to the document-read
input schema definition.
In `@crates/ironclaw_host_runtime/src/memory_binding.rs`:
- Around line 343-349: The override lookup in `memory_binding.rs` is comparing
`extension_id` with raw string equality, so case-only differences like
`acme.honcho` vs `Acme.Honcho` are missed. Update the `has_override` check
inside `requires_certified_bindings()` to normalize both sides to lowercase
using `.to_ascii_lowercase()` before comparing extension names, while keeping
the existing profile and deployment matching logic unchanged.
- Around line 138-144: The redaction logic in redacted_summary uses byte slicing
on extension_id, which can panic for non-ASCII input. Update redacted_summary to
truncate safely by characters/graphemes instead of &ext[..12], preserving the
existing redacted_ext behavior while avoiding any direct byte-index slicing on
external strings.
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 88-96: The host-runtime memory admission path in
`admit_memory_context_snippet` / `admit_memory_context_snippets` currently
trusts provider-returned scope too early, so revalidate each snippet against
`request.scope` and `request.actor` before calling
`admit_memory_context_snippet` or hashing/wrapping it. Add a scope check in the
snippet loop to reject any tenant/user/agent/project mismatch, then only admit
snippets that match the request’s scope and actor. Keep the existing byte limit
and admission flow unchanged for valid snippets.
In `@crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs`:
- Around line 52-68: The current test only validates
`context_retrieve_input_schema_accepts_valid_and_rejects_invalid`, so add
instance-level valid/invalid assertions for the new document-store and
interaction schemas as well. Use the existing `load_schema` and
`jsonschema::validator_for` pattern in this test module to cover
`document-read`, `document-write`, and `interaction-record`, including checks
for required fields, enum values, numeric bounds, and rejection of unexpected
properties. Keep the fixtures alongside the existing schema validation tests so
regressions in those contracts fail in the same suite.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 345-353: `MemoryServiceContextSnippet` is carrying identity
components as raw strings across the service boundary, so replace those fields
with the validated identifier newtypes used by the domain (for example
`TenantId`, `UserId`, `AgentId`, `ProjectId`) or add a typed wrapper before
hashing in `MemoryServiceContextSnippet`/`memory_context.rs`. Update any
constructors, serialization, and call sites so the context can only be built
from validated IDs, preserving the strong contract and preventing malformed or
swapped scope components from changing the snippet reference.
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 2914-2921: The document_store_binding() helper is silently falling
back to MemoryProviderBinding::Native when a supplied MemoryBindingPolicy cannot
resolve the document-store capability or has no matching binding. Change
document_store_binding(policy: Option<&MemoryBindingPolicy>) to return
Result<MemoryProviderBinding, RebornBuildError>, propagate
CapabilityProfileId::new(MEMORY_DOCUMENT_STORE_PROFILE_ID)? instead of dropping
the error, and make the Some(policy) / missing-binding path fail closed with an
error rather than defaulting to Native. Keep only the None policy case as the
native default, and update the call sites to use ? where document_store_binding
is consumed.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3037-3042: The profile source selection in runtime.rs is still
hardwired to the native filesystem path inside the user_profile_source branch,
which bypasses the resolved memory binding. Update the logic around
local_runtime and MemoryBackedUserProfileSourceAdapter to use the same
binding-aware Arc<dyn MemoryService> that memory capabilities use, constructing
MemoryBackedUserProfileSource with that service instead of from_filesystem; if
no valid binding is available, fall back to EmptyUserProfileSource so profile
reads fail closed rather than opening context/profile.json.
---
Outside diff comments:
In `@crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs`:
- Around line 1712-1727: The contract comment in the `BoundaryRule` for
`ironclaw_reborn_cli` still describes the rule as a forbidden-deps list, but the
enforcement now uses an allowlist. Update the wording near the `allowed` field
so it clearly says anything not in `allowed` is rejected, while keeping the same
intent about protecting `ironclaw_reborn_composition`-mediated access to
internal Reborn types.
🪄 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: c15673fc-f28f-4539-939f-0ac535fa233b
📒 Files selected for processing (40)
crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_host_api/src/host_port.rscrates/ironclaw_host_runtime/assets/memory_native/manifest.tomlcrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/context-retrieve.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/context-retrieve.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/interaction-record.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/interaction-record.output.v1.jsoncrates/ironclaw_host_runtime/src/extension_contracts.rscrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/memory_binding.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/memory_native_extension.rscrates/ironclaw_host_runtime/src/memory_profiles.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/memory_capability_profiles.rscrates/ironclaw_host_runtime/tests/memory_native_manifest.rscrates/ironclaw_host_runtime/tests/memory_native_schema_validation.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_host_runtime/tests/user_profile_roundtrip.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_reborn_cli/src/runtime/mod.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/input.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/memory_binding.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_config/src/config_file.rscrates/ironclaw_reborn_config/src/lib.rsdocs/adr/0001-capability-manifest-v2-hard-cutover.mddocs/adr/0002-native-memory-uses-host-storage-ports.mddocs/reborn/contracts/memory-profiles.md
There was a problem hiding this comment.
Actionable comments posted: 2
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/runtime.rs (1)
3018-3042: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale workspace-filesystem contract comment.
Lines 3018-3021 still say profile reads go through the workspace filesystem, but the code now gates on
memory_document_store_bindingand builds fromextension_filesystem. Soften or rewrite that header comment to match the new binding-aware path. As per coding guidelines, comments that promise cross-layer guarantees must match enforced behavior.🤖 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/runtime.rs` around lines 3018 - 3042, The leading workspace-filesystem contract comment is stale and no longer matches the binding-aware implementation. Update the header comment near the profile-source setup in runtime.rs so it describes the current `memory_document_store_binding`-gated path and the use of `extension_filesystem`, and remove or soften any claim that profile reads always go through the workspace filesystem. Keep the surrounding references to `MemoryBackedUserProfileSource`, `EmptyUserProfileSource`, and `identity_context_source` aligned with the actual control flow.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 `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 420-464: Add a caller-level regression in
ProductionMemoryPromptContextService::load_memory_snippets to verify that
snippets with the correct tenant/user but mismatched agent_id or project_id are
filtered out before being returned. Keep the existing
admit_memory_context_snippet helper tests, but add an integration-style test in
the memory_prompt_context test suite that exercises the provider path and
asserts the bad snippet is dropped while matching scope data still passes
through.
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 2928-2948: The missing document-store binding case is only covered
at the helper level, so add a factory-level regression in the build path around
build_reborn_services in factory.rs. Exercise a resolved MemoryBindingPolicy
that omits memory.document_store.v1, then assert the build fails with
RebornBuildError::InvalidConfig from document_store_binding. Use the existing
factory/build symbols to keep the test anchored to the caller and ensure the
invariant is validated end-to-end.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3018-3042: The leading workspace-filesystem contract comment is
stale and no longer matches the binding-aware implementation. Update the header
comment near the profile-source setup in runtime.rs so it describes the current
`memory_document_store_binding`-gated path and the use of
`extension_filesystem`, and remove or soften any claim that profile reads always
go through the workspace filesystem. Keep the surrounding references to
`MemoryBackedUserProfileSource`, `EmptyUserProfileSource`, and
`identity_context_source` aligned with the actual control flow.
🪄 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: be9e9128-9127-4643-a9e3-0a847a478f4b
📒 Files selected for processing (5)
crates/ironclaw_host_runtime/src/memory_binding.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/tests/memory_native_schema_validation.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/runtime.rs
There was a problem hiding this comment.
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/runtime.rs (1)
3018-3021: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale profile-source comment.
This still says profile context comes from
context/profile.jsonvia the workspace filesystem, but the changed code resolves throughMemoryServiceResolverandMemoryService::profile_read. Soften or rewrite these lines to avoid documenting the old provider/path contract.🤖 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/runtime.rs` around lines 3018 - 3021, The comment above the profile resolution flow is stale and still documents the old `context/profile.json` workspace-filesystem contract; update it to match the current `MemoryServiceResolver` and `MemoryService::profile_read` path. Rewrite the text near the profile-source logic so it describes the current in-memory service-based resolution and the graceful `None` fallback without mentioning the deprecated provider or file path.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3018-3021: The comment above the profile resolution flow is stale
and still documents the old `context/profile.json` workspace-filesystem
contract; update it to match the current `MemoryServiceResolver` and
`MemoryService::profile_read` path. Rewrite the text near the profile-source
logic so it describes the current in-memory service-based resolution and the
graceful `None` fallback without mentioning the deprecated provider or file
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e9a1809b-6f40-4c01-aa32-7077341184c9
📒 Files selected for processing (6)
crates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/memory_provider.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/runtime.rs
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json (1)
4-7: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winConstrain
pathto scoped relative form.The schema still accepts absolute and
../traversal paths, so the read tool does not enforce the relative-path contract documented inread.mdand the manifest. Add the same pattern guard here.Based on the manifest wiring and the previous schema review note.
🐛 Suggested schema hardening
"path": { "type": "string", "description": "Relative memory document path to read" + , + "pattern": "^(?!/)(?!.*(?:^|/)\\.\\.(?:/|$))(?!.*\\\\)[A-Za-z0-9._/-]+$" }🤖 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_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json` around lines 4 - 7, The document read input schema still allows unsafe path values; tighten the `path` field in the `memory/document-read.input.v1.json` schema to match the scoped relative-path contract. Update the `path` definition used by the read tool so it rejects absolute paths and `../` traversal, mirroring the same pattern guard used in the manifest and other memory schemas.
🤖 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_host_runtime/assets/memory_native/manifest.toml`:
- Line 20: The manifest has duplicated capability table headers in the
memory_native bundle, which creates empty capability entries before the real
ones. Remove the extra repeated [[capabilities]] headers so each capability
block in manifest.toml maps to only one table, and verify the populated entries
remain under the existing capability sections.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json`:
- Around line 4-40: The document-write schema currently allows invalid mixed or
incomplete requests because it only uses a strict object shape without enforcing
write-mode invariants. Update the schema in document-write.input.v1.json to add
explicit required fields and conditional branches (for plain writes, patch-mode
writes using old_string/new_string/replace_all, and the bootstrap clear path) so
the accepted shapes are mutually exclusive and invalid payloads like empty
objects or mixed modes are rejected at the schema boundary. Use the existing
target, content, append, old_string, new_string, replace_all, and bootstrap
fields as the anchors for these rules.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json`:
- Around line 4-16: The document-write output schema does not actually
discriminate fields by status, so outcome-specific properties can appear or be
missing incorrectly. Update the schema in document-write.output.v1.json to use
status-based validation with oneOf or if/then/else so cleared, patched, and
written each require only their own fields and forbid the others. Keep the
existing status, path, and additionalProperties constraints, and make the rules
explicit around the status enum plus the message, replacements, and append
properties.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json`:
- Around line 4-29: The memory search schema currently advertises alias fields
in search.input.v1.json, but the required constraint still forces query, so
q/text/pattern payloads fail validation. Update the schema so the search
contract accepts at least one of the canonical query inputs (for example via
oneOf/anyOf around query, q, text, and pattern) and clearly define how conflicts
are handled, or remove the alias fields entirely if they are not supported.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.json`:
- Around line 10-17: The search output schema still allows free-form values for
path, which can expose absolute or parent-relative paths to the model. Tighten
the memory/search.output.v1.json schema so the path field uses the same
scoped-relative restriction as the read schema, and keep the existing search
result shape in sync by updating the path property definition in the search
result schema.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.json`:
- Around line 2-4: The tree schema currently uses the untyped items field in
memory/tree.output.v1.json, which removes validation for array elements
entirely. Update the schema so the array items are constrained to the actual
node shape used by the memory tree, ideally with a typed object or union that
distinguishes document vs directory entries, and keep the schema aligned with
the runtime types referenced by the tree output contract.
In `@crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml`:
- Around line 117-126: Remove the default network grant from the live
ironclaw.memory.native capability entries in local_dev_capability_policy.toml so
the native memory search/write grants match the filesystem-only trust model.
Update the relevant grants for ironclaw.memory.native.search and
ironclaw.memory.native.write to keep only the dispatch/filesystem authority
already reflected in factory.rs and runtime/local_dev.rs, and ensure no network
= "default" remains for these always-on memory tools.
---
Duplicate comments:
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json`:
- Around line 4-7: The document read input schema still allows unsafe path
values; tighten the `path` field in the `memory/document-read.input.v1.json`
schema to match the scoped relative-path contract. Update the `path` definition
used by the read tool so it rejects absolute paths and `../` traversal,
mirroring the same pattern guard used in the manifest and other memory schemas.
🪄 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: a99c0ebd-432e-42a6-8503-2dd5dab92f8c
📒 Files selected for processing (25)
crates/ironclaw_host_runtime/assets/memory_native/manifest.tomlcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.mdcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/search.mdcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/tree.mdcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/write.mdcrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.jsoncrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/memory_native_extension.rscrates/ironclaw_host_runtime/src/surface.rscrates/ironclaw_host_runtime/tests/memory_native_manifest.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/local_dev_capability_policy.tomlcrates/ironclaw_reborn_composition/src/runtime/local_dev.rsdocs/adr/0002-native-memory-uses-host-storage-ports.mddocs/reborn/contracts/memory-profiles.md
5 tasks
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/service.rs`:
- Around line 74-84: The `target` fallback in `service.rs` is currently
unreachable on the schema-validated path because `document-write.input.v1.json`
still only allows a string. Update the schema and the
`parse_write_command`/input parsing logic together so `target: null` is either
accepted intentionally or removed from the compatibility handling, and then add
a regression test covering the intended `target` behavior through the
schema-driven flow.
In `@tests/support/reborn/harness.rs`:
- Around line 2078-2094: The native-memory trust tuple added for
core_builtin_tools_from_runtime is still missing from
HostRuntimeCapabilityHarness::qa_smoke_tools(), where the MEMORY_* capability
IDs are still wired to builtin with no additional_provider_trust. Extract the
new trust tuple into a shared helper and reuse it in both harness constructors
so qa_smoke_tools() also trusts
ExtensionId::new(NATIVE_MEMORY_FIRST_PARTY_PROVIDER) with the same
DispatchCapability, ReadFilesystem, and WriteFilesystem effects. This keeps
create_capability_port() authorization aligned with the split provider model and
prevents QA-smoke memory grants from failing closed as untrusted.
🪄 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: 8ab5cbdb-b69d-41e2-a595-8e13de85095f
📒 Files selected for processing (7)
crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.jsoncrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/memory_native_schema_validation.rscrates/ironclaw_memory/src/service.rstests/reborn_trace_first_party_tool_coverage.rstests/support/reborn/harness.rs
github-actions
Bot
added
scope: dependencies
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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_architecture/tests/reborn_dependency_boundaries.rs`:
- Around line 1599-1611: The `ironclaw_memory_mem0` boundary entry only enforces
its allowed dependencies and does not actually prevent `ironclaw_host_runtime`
from naming this crate. Add an explicit boundary rule for
`ironclaw_host_runtime` in `reborn_dependency_boundaries.rs` that excludes
`ironclaw_memory_mem0`, or revise the nearby comment to describe the restriction
as intended rather than enforced. Use the existing `BoundaryRule` entries in the
test file to keep the new rule consistent with the other crate boundary
definitions.
In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 186-205: The mem0 read and profile paths only consume the first
list page, so results beyond page 1 are missed. Update the list-fetch logic in
the relevant service methods in service.rs to keep following mem0 pagination
using the page/next fields until all memories are collected, then rebuild the
document/profile from the full set. Apply this to the code paths that perform
the list call and reconstruct results so reads and profile_read do not stop at
the first page.
In `@crates/ironclaw_memory_mem0/src/transport.rs`:
- Around line 201-205: The mem0 body parsing in the transport boundary currently
hides malformed JSON by using `serde_json::from_str(&text).unwrap_or_default()`,
which silently swallows parse errors. Update the body handling in `transport.rs`
to either propagate the `serde_json::from_str` failure from this external IO
boundary or replace it with an explicitly documented tolerated fallback marked
with a `silent-ok` rationale and a log that records only the parse error; keep
the `body` construction in the same `let body = if text.trim().is_empty()` flow.
- Around line 157-160: The reqwest client built in Mem0HttpTransport::new is
missing a timeout, so add a bounded client timeout directly in the
reqwest::Client::builder chain before build() and keep the existing error
mapping intact. Use the existing client construction path in transport.rs to
ensure all mem0 HTTP requests inherit the limit rather than relying on upstream
callers.
In `@crates/ironclaw_memory_mem0/src/url_check.rs`:
- Around line 28-57: Reject non-path URL components in check_base_url: the
current reqwest::Url validation in url_check.rs still allows credentials, query
strings, and fragments in Mem0 base URLs. Update check_base_url to fail closed
when parsed.username()/parsed.password() are present, or when parsed.query() or
parsed.fragment() is set, and return Mem0Error::InvalidUrl with a clear reason.
Keep the existing scheme/host/IP checks intact while enforcing this stricter
boundary on base URLs.
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 3284-3287: The startup path is currently failing open when the
selected memory provider cannot be constructed, so update the
`build_memory_service_resolver()` flow used in `factory.rs` to fail closed
instead of returning a resolver without the third-party provider. Make the
resolver construction or the surrounding production validation return a
`Result`, and map missing or rejected provider handles from
`MemoryProviderDeps::for_third_party(...)` to `RebornBuildError::InvalidConfig`
before reporting ready. Apply the same fix to both call sites that build
`memory_resolver`, so production and migration-dry-run profiles refuse startup
when the selected memory provider is unavailable.
In `@crates/ironclaw_reborn_composition/src/lib.rs`:
- Around line 231-234: The crate root is re-exporting internal provider factory
seams instead of only facade-shaped composition APIs. Remove the public exposure
of MemoryProviderDeps, Mem0ConnectionConfig, and create_document_store_provider
from the lib.rs re-export, and keep them private or pub(crate) within
memory_provider_factory; if tests need access, gate that access behind
test-support rather than the production crate root. Keep
build_memory_service_resolver as the only surfaced composition entrypoint if it
is the intended facade.
🪄 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: 76613def-41db-434d-8a35-094f1930e00d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (20)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/memory_provider.rscrates/ironclaw_memory_mem0/Cargo.tomlcrates/ironclaw_memory_mem0/src/config.rscrates/ironclaw_memory_mem0/src/error.rscrates/ironclaw_memory_mem0/src/lib.rscrates/ironclaw_memory_mem0/src/service.rscrates/ironclaw_memory_mem0/src/transport.rscrates/ironclaw_memory_mem0/src/url_check.rscrates/ironclaw_reborn_cli/src/runtime/mod.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_binding.rscrates/ironclaw_reborn_composition/src/memory_provider_factory.rscrates/ironclaw_reborn_composition/tests/memory_mem0_swap.rscrates/ironclaw_reborn_config/src/config_file.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_memory_mem0/src/service.rs (1)
236-246: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSort document parts before concatenating mem0 reads.
Line 246 joins matched memories in backend list order, but Line 454 notes mem0/Qdrant listing is not chronological. Multiple writes to the same
targetcan therefore read back in nondeterministic order.Suggested fix
- let parts: Vec<String> = response_items(&response.body) + let mut parts: Vec<(String, usize, String)> = response_items(&response.body) .into_iter() + .enumerate() - .filter(|item| item_metadata_str(item, TARGET_KEY) == Some(request.path.as_str())) - .filter_map(|item| item_text(item).map(str::to_string)) + .filter(|(_, item)| item_metadata_str(item, TARGET_KEY) == Some(request.path.as_str())) + .filter_map(|(index, item)| { + item_text(item) + .map(|text| (item_created_at(item).to_string(), index, text.to_string())) + }) .collect(); if parts.is_empty() { return Err(MemoryServiceError::input()); } - let content = parts.join("\n"); + parts.sort_by(|(left_created_at, left_index, _), (right_created_at, right_index, _)| { + left_created_at + .cmp(right_created_at) + .then(left_index.cmp(right_index)) + }); + let content = parts + .into_iter() + .map(|(_, _, part)| part) + .collect::<Vec<_>>() + .join("\n");Also applies to: 454-456
🤖 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_mem0/src/service.rs` around lines 236 - 246, The reconstructed document in the mem0 read path is joined in backend list order, but that order is not guaranteed to be chronological. Update the logic in the response processing block that builds `parts` from `response_items(&response.body)` so the matched memories are sorted deterministically before `join("\n")`, using the available metadata/timestamp fields from the item payload. Apply the same ordering assumption consistently with the note in the later mem0/Qdrant listing logic so repeated writes to the same `target` read back in a stable order.
🤖 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_mem0/src/service.rs`:
- Around line 122-142: The fallback in fetch_latest_profile_object is silently
discarding malformed profile JSON and turning it into an empty object, which can
cause profile_set to overwrite existing fields. Update this method to
distinguish “no prior profile” from “parse failed” by propagating serde_json
parse errors as MemoryServiceError instead of using unwrap_or_default. Keep the
empty-object fallback only for genuinely missing profile data from
latest_profile_text/response_items, and ensure any intentional silent fallback
is explicitly justified with a silent-ok comment if retained.
In `@crates/ironclaw_reborn_config/src/config_file.rs`:
- Around line 122-131: Validate memory.mem0_base_url alongside the other
operator-supplied string fields in the config validation path, not just bindings
and overrides. Update the validation logic in config_file.rs so the
mem0_base_url value is trimmed and checked for inline-secret/private-URL
patterns the same way the existing operator-supplied strings are handled, and
reject whitespace-only or credential-bearing URLs before provider startup. Use
the existing config validation helpers and the mem0_base_url field on the config
struct as the main points to update.
---
Outside diff comments:
In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 236-246: The reconstructed document in the mem0 read path is
joined in backend list order, but that order is not guaranteed to be
chronological. Update the logic in the response processing block that builds
`parts` from `response_items(&response.body)` so the matched memories are sorted
deterministically before `join("\n")`, using the available metadata/timestamp
fields from the item payload. Apply the same ordering assumption consistently
with the note in the later mem0/Qdrant listing logic so repeated writes to the
same `target` read back in a stable order.
🪄 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: a49e4842-2956-4e21-a069-008dc1fef1b0
📒 Files selected for processing (8)
crates/ironclaw_memory_mem0/src/lib.rscrates/ironclaw_memory_mem0/src/service.rscrates/ironclaw_memory_mem0/src/transport.rscrates/ironclaw_memory_mem0/tests/live_local_mem0.rscrates/ironclaw_reborn_cli/src/runtime/mod.rscrates/ironclaw_reborn_composition/src/memory_provider_factory.rscrates/ironclaw_reborn_composition/tests/memory_mem0_swap.rscrates/ironclaw_reborn_config/src/config_file.rs
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
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_cli/src/runtime/mod.rs (1)
693-702: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail loud on unreadable mem0 env settings.
optional_nonempty_env()usesstd::env::var(name).ok(), soNotUnicodeis treated as “unset”. ForMEMORY_MEM0_BASE_URLthis can fall back to config, and forMEMORY_MEM0_API_KEYit silently drops auth. DistinguishNotPresentfrom unreadable values and propagate the latter.As per path instructions, “Fail loud” flags silent boundary/settings failures.
Suggested fix
- let mem0_base_url = optional_nonempty_env("MEMORY_MEM0_BASE_URL").or_else(|| { + let mem0_base_url = optional_nonempty_env("MEMORY_MEM0_BASE_URL")?.or_else(|| { config_file .as_ref() .and_then(|file| file.memory.as_ref()) .and_then(|memory| memory.mem0_base_url.clone()) }); let memory_provider_connection = ironclaw_reborn_composition::Mem0ConnectionConfig { base_url: mem0_base_url, - api_key: optional_nonempty_env("MEMORY_MEM0_API_KEY").map(SecretString::from), - app_id: optional_nonempty_env("MEMORY_MEM0_APP_ID"), + api_key: optional_nonempty_env("MEMORY_MEM0_API_KEY")?.map(SecretString::from), + app_id: optional_nonempty_env("MEMORY_MEM0_APP_ID")?, };-fn optional_nonempty_env(name: &str) -> Option<String> { - std::env::var(name) - .ok() - .map(|value| value.trim().to_string()) - .filter(|value| !value.is_empty()) +fn optional_nonempty_env(name: &str) -> anyhow::Result<Option<String>> { + match std::env::var(name) { + Ok(value) => Ok(Some(value.trim().to_string()).filter(|value| !value.is_empty())), + Err(std::env::VarError::NotPresent) => Ok(None), + Err(std::env::VarError::NotUnicode(_)) => { + Err(anyhow::anyhow!("{name} must be valid UTF-8")) + } + } }🤖 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_cli/src/runtime/mod.rs` around lines 693 - 702, The mem0 environment lookup is treating unreadable values as missing, which causes silent fallback for MEMORY_MEM0_BASE_URL and drops MEMORY_MEM0_API_KEY auth data. Update optional_nonempty_env (or add a dedicated helper used here in runtime/mod.rs) to distinguish NotPresent from NotUnicode and return an error for unreadable env vars instead of converting them to None. Then have the Mem0ConnectionConfig construction for memory_provider_connection propagate that error so invalid settings fail loudly rather than falling back to config or empty credentials.Source: Path instructions
🤖 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.
Outside diff comments:
In `@crates/ironclaw_reborn_cli/src/runtime/mod.rs`:
- Around line 693-702: The mem0 environment lookup is treating unreadable values
as missing, which causes silent fallback for MEMORY_MEM0_BASE_URL and drops
MEMORY_MEM0_API_KEY auth data. Update optional_nonempty_env (or add a dedicated
helper used here in runtime/mod.rs) to distinguish NotPresent from NotUnicode
and return an error for unreadable env vars instead of converting them to None.
Then have the Mem0ConnectionConfig construction for memory_provider_connection
propagate that error so invalid settings fail loudly rather than falling back to
config or empty credentials.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 55a7d7c2-3186-44d1-8944-01f81c13a281
📒 Files selected for processing (2)
crates/ironclaw_reborn_cli/src/runtime/mod.rscrates/ironclaw_reborn_config/src/config_file.rs
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (7)
crates/ironclaw_host_runtime/src/memory_binding.rs (1)
347-353: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize override extension ids before matching.
Line 350 still does a byte-for-byte comparison between the bound
extension_idand the admin override. A case-only difference likeacme.honchovsAcme.Honchomakes production resolution reject the override and fail boot even though the extension id is semantically the same. Lowercase both sides once before theany(...)check. As per path instructions, "When comparing user-supplied strings ... extension names, normalize to lowercase with.to_ascii_lowercase()for case-insensitive comparison."🤖 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_host_runtime/src/memory_binding.rs` around lines 347 - 353, The override lookup in memory_binding’s certified-bindings path compares extension_id byte-for-byte, so case-only differences can prevent a valid admin override from matching. Normalize both the bound extension_id and each override.extension_id to lowercase with to_ascii_lowercase() before the any(...) check in the requires_certified_bindings block, keeping the existing profile_id and deployment_profile matching logic unchanged.Source: Path instructions
crates/ironclaw_memory_mem0/src/transport.rs (1)
223-227: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not hide malformed mem0 response bodies.
Lines 223-227 still collapse
serde_json::from_str(&text)failures into a normalValue::Nullresponse. That masks upstream protocol breaks at an external IO boundary and makes them indistinguishable from an actually empty body. Either propagate the parse error, or keep the null fallback only with an explicit// silent-ok: ...rationale and a debug log of the parse failure. As per path instructions, "Fail loud: flag silent-failure patterns," and as per coding guidelines, "When a fallback is genuinely acceptable, it must be justified inline with a// silent-ok: <reason>comment naming the operation."🤖 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_mem0/src/transport.rs` around lines 223 - 227, The mem0 response parsing in transport.rs is hiding malformed JSON by falling back to Value::Null in the body construction. Update the response handling around the serde_json::from_str path to either propagate the parse error from the transport boundary or, if null is truly acceptable, add an inline // silent-ok: reason comment naming the parse operation and emit a debug log with the parse failure. Keep the change localized to the body parsing logic so empty bodies and malformed bodies are no longer indistinguishable.Sources: Coding guidelines, Path instructions
crates/ironclaw_reborn_composition/src/lib.rs (1)
231-234: 📐 Maintainability & Code Quality | 🟠 Major | ⚖️ Poor tradeoffCrate root still re-exports provider-factory seams.
MemoryProviderDeps,Mem0ConnectionConfig, andcreate_document_store_providerare wiring/test seams, not facade-shaped composition API. As per coding guidelines (ironclaw_reborn_composition: expose facade-shaped handles only; keep lower substrate handles private) and path instructions (nopub usere-exports unless for downstream consumers), keep thesepub(crate)and gate any integration-test access behindtest-support.build_memory_service_resolveris the intended facade entrypoint.🤖 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/lib.rs` around lines 231 - 234, The crate root is re-exporting provider-factory test/wiring seams that should stay private. Remove the public re-exports of MemoryProviderDeps, Mem0ConnectionConfig, and create_document_store_provider from the lib.rs facade, keep them pub(crate) inside memory_provider_factory, and expose any test-only access through test-support instead; keep build_memory_service_resolver as the only public composition entrypoint.Sources: Coding guidelines, Path instructions
crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml (1)
117-144: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDrop
network = "default"from the live native-memory grants.The live
ironclaw.memory.nativeprovider is filesystem-backed only in this PR (the trust entries infactory.rsgrant only dispatch + read/write filesystem). Keepingnetwork = "default"onsearch/write/read/treehands the always-on memory tools broader authority than the implementation needs. As per coding guidelines (crates/**/*.rs: fail closed for network policy).Suggested fix
capability = "ironclaw.memory.native.search" effects = ["dispatch_capability", "read_filesystem"] mounts = "memory" -network = "default" @@ capability = "ironclaw.memory.native.write" effects = ["dispatch_capability", "read_filesystem", "write_filesystem"] mounts = "memory" -network = "default" @@ capability = "ironclaw.memory.native.read" effects = ["dispatch_capability", "read_filesystem"] mounts = "memory" -network = "default" @@ capability = "ironclaw.memory.native.tree" effects = ["dispatch_capability", "read_filesystem"] mounts = "memory" -network = "default"🤖 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/local_dev_capability_policy.toml` around lines 117 - 144, The live native-memory grants in the local dev policy are over-permissive by still setting network = "default" on the ironclaw.memory.native.search, ironclaw.memory.native.write, ironclaw.memory.native.read, and ironclaw.memory.native.tree entries. Remove the network setting from these native-memory grant blocks in local_dev_capability_policy.toml so they match the filesystem-only trust model used by factory.rs and fail closed for network access.Source: Coding guidelines
crates/ironclaw_reborn_cli/src/runtime/mod.rs (1)
693-704: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winConfig-sourced
mem0_base_urlreaches the connection unchecked.The env path runs through
optional_nonempty_env(trim + non-empty), but the[memory].mem0_base_urlfallback is cloned verbatim — no trim, no inline-secret reject. The root cause is the missing config-layer validation flagged inconfig_file.rs; fixing it there closes this path too.check_base_urlin the transport is a last line of defense, not a substitute for the secrets guard.🤖 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_cli/src/runtime/mod.rs` around lines 693 - 704, The config fallback for Mem0 base URL is bypassing the same validation applied to env values, so fix the config-layer handling in the `config_file` path and ensure `[memory].mem0_base_url` is trimmed, non-empty validated, and rejected if it contains inline secrets before it reaches `Mem0ConnectionConfig`. Keep `optional_nonempty_env` as-is for env vars, but update the config parsing/validation logic so `memory.mem0_base_url` is normalized and checked centrally, then let `runtime/mod.rs` continue wiring it into `with_memory_provider_connection` unchanged.crates/ironclaw_reborn_composition/src/factory.rs (1)
3284-3287: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftProduction startup fails open when the bound memory provider can't be built.
build_memory_service_resolverlogs and returns a resolver without the third-party provider when mem0 URL/credentials are missing or rejected, so production reports ready while configured memory dispatch fails closed only at invocation time. Per coding guidelines (ironclaw_reborn_composition: production/migration-dry-run must fail closed on missing required handles), convert this to aResultor add a production validation step mapping a missing-selected-provider handle toRebornBuildError::InvalidConfig. Also applies to Lines 3324-3327 (Postgres arm).🤖 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 3284 - 3287, The resolver setup in `build_memory_service_resolver` is allowing startup to succeed even when the selected third-party memory provider cannot be constructed, so update the `memory_resolver` path to fail closed by propagating a `Result` or validating the selected handle and converting missing/invalid production config into `RebornBuildError::InvalidConfig`. Apply the same production validation to the Postgres branch as well, and use the existing `MemoryProviderDeps::for_third_party` and `build_memory_service_resolver` flow as the entry points for the fix so the unsupported/missing provider is rejected before readiness is reported.Source: Coding guidelines
crates/ironclaw_reborn_config/src/config_file.rs (1)
945-980: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
memory.mem0_base_urlskips the inline-secret/trim guard.The
memoryvalidation arm checks bindings/overrides but never touchesmem0_base_url(Line 132). A credential-bearing or whitespace-only URL pasted here bypassesreject_inline_secret/trim and only fails later at transport construction. As per coding guidelines (crates/**/*.rs: do not expose raw secrets or private URLs across public surfaces).Suggested fix
if let Some(memory) = &self.memory { + if let Some(mem0_base_url) = &memory.mem0_base_url { + check_non_empty_trimmed(Cow::Borrowed("memory.mem0_base_url"), mem0_base_url)?; + } for (idx, binding) in memory.profile_bindings.iter().enumerate() {🤖 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_config/src/config_file.rs` around lines 945 - 980, The `memory` validation block in `config_file.rs` validates `profile_bindings` and `admin_overrides` but skips `mem0_base_url`, allowing whitespace-only or secret-like values to bypass the same trim/inline-secret checks. Update the `if let Some(memory) = &self.memory` section to validate `memory.mem0_base_url` with the existing `check_non_empty_trimmed` and `reject_inline_secret` helpers before validating the bindings/overrides, using the same `memory.mem0_base_url` field path pattern as the other fields.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
`@crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.md`:
- Around line 1-3: The `read` prompt is incorrectly implying it can return
profile facts, but profile data is accessed through
`MemoryService::profile_read` and not the path-based document `read(path)`
surface. Update the `read.md` wording to describe only persistent document
content for notes/logs/etc., and remove “profile facts” so the prompt stays
aligned with `user_profile_source.rs` and the `MemoryService::profile_read` /
`builtin.profile_set` contract.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.json`:
- Around line 4-8: The output schema for the memory document read response
currently leaves word_count optional, even though NativeMemoryService::read()
and the mem0 read() path always populate it through MemoryServiceReadResponse.
Update the document-read.output.v1.json schema so word_count is included in the
required fields alongside path and content, and keep the field definition
aligned with the existing schema entry to enforce the tool contract.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json`:
- Around line 10-13: The schema descriptions in the document-write output no
longer match the shared document-store contract, so update the
native/profile-level schema to reflect the actual statuses returned by the write
flow. In the document-write output schema and the profile registration in
memory_profiles, revise the field docs for
message/replacements/append/content_length so they describe all valid write
outcomes from the service.rs write path, or split the native-specific and shared
contracts if the meanings differ. Use the document-write.output.v1 schema and
the memory.document_store.v1 profile as the key places to align.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json`:
- Around line 4-18: Tighten the search request schema in the memory search input
JSON so empty strings and ambiguous alias combinations are rejected at the
boundary. Update the schema around the query-related fields (`query`, `q`,
`text`, `pattern`) to require non-empty values and enforce exactly one alias
key, or else clearly define canonical precedence directly in the schema. Use the
existing search input schema definition to make the validation unambiguous
before downstream normalization.
In `@crates/ironclaw_host_runtime/src/user_profile_source.rs`:
- Around line 50-57: The `from_filesystem` helper in `UserProfileSource` is
bypassing the configured document-store binding by always constructing
`NativeMemoryService` directly. Update this path to use the already-resolved
`Arc<dyn MemoryService>` from `memory_provider`’s fail-closed selection logic,
or rename/restrict `from_filesystem` so it is explicitly native-only and cannot
be used as a generic memory-capability source. Ensure `UserProfileSource::new`
receives the bound `MemoryService` so profile reads and writes stay on the same
backend as `profile_set`.
In `@crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs`:
- Around line 8612-8617: The native-memory trust wiring in this test harness is
incomplete: trust_policy() is still granting ironclaw.memory.native the full
builtin_effects() ceiling, and provider_trust() only maps builtin, so
CapabilityCatalog::visible_capabilities can omit native providers and hide
least-privilege regressions. Update the trust helpers in
first_party_builtin_tools.rs so native memory has its own trust entry and a
narrower policy derived from its manifest’s declared filesystem effects, and
ensure provider_trust() includes ironclaw.memory.native alongside builtin so
surface tests expose native descriptors correctly.
In `@crates/ironclaw_memory_mem0/src/error.rs`:
- Around line 21-22: The `InvalidUrl` error in `error.rs` is leaking the
configured mem0 URL through its `Display` message. Update the `thiserror`
variant so `InvalidUrl` keeps the `reason` but omits or redacts the raw `url`
value, and make sure any caller constructing or logging this error no longer
exposes the configured URL on public error surfaces.
In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 181-218: The `MemoryService::write` path is silently treating
non-append writes as adds by always calling `Mem0HttpRequest::post(ADD_PATH,
...)` and returning `append: true`. Update `write` to explicitly reject any
request where `request.append` is false, alongside the existing unsupported
patch/empty-content checks, and return an operation/unsupported error instead of
proceeding. Keep the behavior aligned with the other fail-closed branches in
`write`, using the `MemoryServiceWriteRequest` fields and
`MemoryServiceError::operation_from` / `Mem0Error::Unsupported` for the
unsupported case.
- Around line 236-246: The document reconstruction in the mem0 service is using
raw response order, which can scramble multiple fragments for the same target.
Update the logic in response_items/item_metadata_str/item_text handling to sort
the matching items by created_at before building the final content, and use a
stable fallback ordering for items without timestamps so append order is
preserved. Keep the existing filtering by TARGET_KEY and join only after the
ordered list is produced, so the round-trip behavior promised by the module docs
is enforced.
In `@crates/ironclaw_reborn_composition/src/memory_provider_factory.rs`:
- Around line 119-123: The selected mem0 binding can currently be left active
even when no provider was created, because create_document_store_provider and
the resolver wiring in build_memory_service_resolver collapse unsupported ids,
missing config, and transport failures into None. Make this path fallible
instead: return an error when the chosen MemoryProviderBinding cannot be
realized, and propagate that error out of build_memory_service_resolver so
startup fails with context rather than registering a half-initialized resolver.
Use the existing create_document_store_provider and
MemoryProviderBinding/MemoryService symbols to locate the affected flow, and
avoid warn-and-continue behavior that leaves a selected binding without a
provider.
In `@docs/reborn/contracts/memory-profiles.md`:
- Around line 96-100: Update the Non-goals section so it no longer states there
is no Honcho/mem0 provider implementation, since the new mem0 crate and
composition wiring/tests already bind memory.document_store.v1 to mem0. Reword
that bullet to reflect the actual intent, such as that mem0 is not the default
or not certified, and keep the other non-goals unchanged.
---
Duplicate comments:
In `@crates/ironclaw_host_runtime/src/memory_binding.rs`:
- Around line 347-353: The override lookup in memory_binding’s
certified-bindings path compares extension_id byte-for-byte, so case-only
differences can prevent a valid admin override from matching. Normalize both the
bound extension_id and each override.extension_id to lowercase with
to_ascii_lowercase() before the any(...) check in the
requires_certified_bindings block, keeping the existing profile_id and
deployment_profile matching logic unchanged.
In `@crates/ironclaw_memory_mem0/src/transport.rs`:
- Around line 223-227: The mem0 response parsing in transport.rs is hiding
malformed JSON by falling back to Value::Null in the body construction. Update
the response handling around the serde_json::from_str path to either propagate
the parse error from the transport boundary or, if null is truly acceptable, add
an inline // silent-ok: reason comment naming the parse operation and emit a
debug log with the parse failure. Keep the change localized to the body parsing
logic so empty bodies and malformed bodies are no longer indistinguishable.
In `@crates/ironclaw_reborn_cli/src/runtime/mod.rs`:
- Around line 693-704: The config fallback for Mem0 base URL is bypassing the
same validation applied to env values, so fix the config-layer handling in the
`config_file` path and ensure `[memory].mem0_base_url` is trimmed, non-empty
validated, and rejected if it contains inline secrets before it reaches
`Mem0ConnectionConfig`. Keep `optional_nonempty_env` as-is for env vars, but
update the config parsing/validation logic so `memory.mem0_base_url` is
normalized and checked centrally, then let `runtime/mod.rs` continue wiring it
into `with_memory_provider_connection` unchanged.
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 3284-3287: The resolver setup in `build_memory_service_resolver`
is allowing startup to succeed even when the selected third-party memory
provider cannot be constructed, so update the `memory_resolver` path to fail
closed by propagating a `Result` or validating the selected handle and
converting missing/invalid production config into
`RebornBuildError::InvalidConfig`. Apply the same production validation to the
Postgres branch as well, and use the existing
`MemoryProviderDeps::for_third_party` and `build_memory_service_resolver` flow
as the entry points for the fix so the unsupported/missing provider is rejected
before readiness is reported.
In `@crates/ironclaw_reborn_composition/src/lib.rs`:
- Around line 231-234: The crate root is re-exporting provider-factory
test/wiring seams that should stay private. Remove the public re-exports of
MemoryProviderDeps, Mem0ConnectionConfig, and create_document_store_provider
from the lib.rs facade, keep them pub(crate) inside memory_provider_factory, and
expose any test-only access through test-support instead; keep
build_memory_service_resolver as the only public composition entrypoint.
In `@crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml`:
- Around line 117-144: The live native-memory grants in the local dev policy are
over-permissive by still setting network = "default" on the
ironclaw.memory.native.search, ironclaw.memory.native.write,
ironclaw.memory.native.read, and ironclaw.memory.native.tree entries. Remove the
network setting from these native-memory grant blocks in
local_dev_capability_policy.toml so they match the filesystem-only trust model
used by factory.rs and fail closed for network access.
In `@crates/ironclaw_reborn_config/src/config_file.rs`:
- Around line 945-980: The `memory` validation block in `config_file.rs`
validates `profile_bindings` and `admin_overrides` but skips `mem0_base_url`,
allowing whitespace-only or secret-like values to bypass the same
trim/inline-secret checks. Update the `if let Some(memory) = &self.memory`
section to validate `memory.mem0_base_url` with the existing
`check_non_empty_trimmed` and `reject_inline_secret` helpers before validating
the bindings/overrides, using the same `memory.mem0_base_url` field path pattern
as the other fields.
🪄 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: 3042703e-1241-405f-96ad-8d60d5af5069
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (66)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_host_api/src/host_port.rscrates/ironclaw_host_runtime/assets/memory_native/manifest.tomlcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.mdcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/search.mdcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/tree.mdcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/write.mdcrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.jsoncrates/ironclaw_host_runtime/src/extension_contracts.rscrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/memory_binding.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/memory_native_extension.rscrates/ironclaw_host_runtime/src/memory_profiles.rscrates/ironclaw_host_runtime/src/memory_provider.rscrates/ironclaw_host_runtime/src/surface.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/memory_capability_profiles.rscrates/ironclaw_host_runtime/tests/memory_native_manifest.rscrates/ironclaw_host_runtime/tests/memory_native_schema_validation.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_host_runtime/tests/user_profile_roundtrip.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_mem0/Cargo.tomlcrates/ironclaw_memory_mem0/src/config.rscrates/ironclaw_memory_mem0/src/error.rscrates/ironclaw_memory_mem0/src/lib.rscrates/ironclaw_memory_mem0/src/service.rscrates/ironclaw_memory_mem0/src/transport.rscrates/ironclaw_memory_mem0/src/url_check.rscrates/ironclaw_memory_mem0/tests/live_local_mem0.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_reborn_cli/src/runtime/mod.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/local_dev_capability_policy.tomlcrates/ironclaw_reborn_composition/src/memory_binding.rscrates/ironclaw_reborn_composition/src/memory_provider_factory.rscrates/ironclaw_reborn_composition/src/projection/display_preview.rscrates/ironclaw_reborn_composition/src/projection/tests/display_preview.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/tests/memory_mem0_swap.rscrates/ironclaw_reborn_config/src/config_file.rscrates/ironclaw_reborn_config/src/lib.rsdocs/adr/0001-capability-manifest-v2-hard-cutover.mddocs/adr/0002-native-memory-uses-host-storage-ports.mddocs/reborn/contracts/memory-profiles.mdtests/reborn_trace_first_party_tool_coverage.rstests/support/reborn/harness.rs
…ce-wide allowlist PR #5163 made only the memory rules allowlist-based; the other ~30 `BoundaryRule` entries stayed blocklists that under-enforce — they forbid today's offenders but would silently admit a future internal dep (e.g. `ironclaw_turns`, `ironclaw_product_workflow`, `ironclaw_reborn`). Convert the whole harness to an allowlist: - `BoundaryRule` now carries `allowed: Vec<&'static str>` instead of `forbidden`. The runner computes `forbidden = workspace_ironclaw_crates() - allowed - crate_name`, so any unlisted internal dependency now fails the boundary test. - Each crate's `allowed` set is its actual normal `ironclaw_*` dependencies, matching the dependency guardrail documented in its CLAUDE.md/AGENTS.md. - The three previously-inline allowlist rules (`ironclaw_host_api`, `ironclaw_memory`, `ironclaw_memory_native`) are folded into `boundary_rules()` so the test body is a single uniform loop. Behavior-preserving on the current (correct) dependency graph; the win is forward enforcement. Addresses serrrfirat's deferred thread on #5163 (discussion_r3468163078). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ontext Before this change the native provider sanitized, wrapped, and hashed each memory snippet, and the host only *asserted* the `Untrusted memory content:` prefix in `admit_memory_context_snippet`. A future untrusted provider could pre-attach that prefix (or pre-shape the snippet) and slip text past the host's prompt-safety wrapper. Move all model-visible shaping into the host so the provider can never bypass prompt safety: - `MemoryServiceContextSnippet` now carries RAW snippet text plus the resolved scope/path components (`tenant_id`, `user_id`, `agent_id`, `project_id`, `relative_path`) — no `snippet_ref`/`safe_summary`/`model_content`. - The native `retrieve_context` ranks and scope-filters candidates, then returns them raw; it no longer sanitizes, truncates, hashes, or budgets. Removed the native `sanitize_snippet_text`, `truncate_to_char_boundary`, `validate_loop_safe_summary`, `memory_snippet_display_ref`, `feed_hash`, `collect_context_snippets`, the FNV/budget consts, and the prompt-envelope dep. - The host `memory_context.rs` builds the `memory-snippet:*` reference via the canonical `ironclaw_turns::run_profile::memory_snippet_display_ref`, sanitizes + wraps the raw text (`sanitize_snippet_text` relocated here), validates through the loop's own `LoopSafeSummary` gate (collapsing the native denylist copy into one source of truth), and enforces the per-snippet (512B) + aggregate (4 KiB) budgets in the admission loop with the same break semantics the native `collect_context_snippets` used. Behavior-preserving for the native provider: model-visible output is byte-for-byte identical (same wrapping, same FNV trailing-separator `memory-snippet:cb96ed00b13e6ae4` golden ref, same caps/ordering). New coverage proves a provider that returns text merely starting with the untrusted prefix is STILL re-sanitized + re-wrapped by the host (`adapter_re_sanitizes_provider_supplied_untrusted_prefix`, `sanitize_re_wraps_text_already_carrying_untrusted_prefix`), plus the legacy ref golden lock and the host-owned aggregate-budget test. Addresses serrrfirat's deferred security thread on #5163 (discussion_r3468163070; ref-stability discussion_r3466587649). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…facade Profile READS built the native repository/backend directly in `user_profile_source.rs` and duplicated the scope/path decision (`profile_scope_and_path` + `PROFILE_DOCUMENT_PATH`) that `NativeMemoryService` already owns for WRITES (`profile_set`). That coupled profile reads to the concrete native provider and left provider selection unable to swap reads with the rest of the memory facade. Add a provider-neutral profile read to the contract and route the host read through it: - New `MemoryService::profile_read(invocation) -> MemoryServiceProfileReadResponse` (raw document bytes) on the `ironclaw_memory` trait, with a native implementation that reuses the SAME `profile_scope_and_path` as `profile_set` — so the scope/path decision lives in exactly one place per provider. - `MemoryBackedUserProfileSource` now holds `Arc<dyn MemoryService>` and reads via `profile_read`; the host keeps only the parse + 64 KiB size-cap + validation. Deleted the duplicate host `profile_scope_and_path` / `PROFILE_DOCUMENT_PATH` and their re-export. - Production wiring uses the new `MemoryBackedUserProfileSource::from_filesystem` factory (host owns the native-provider choice, matching the memory capability); the composition layer keeps passing the workspace filesystem. Behavior-preserving: the unit tests assert identical parse/validation outcomes (now through a stub `MemoryService`), and the end-to-end `user_profile_roundtrip` test still proves the agent-scoped write → user-scoped read round trip, now through `MemoryService::profile_read`. Addresses serrrfirat's deferred thread on #5163 (discussion_r3466587663). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5205
July 6, 2026 14:13
Destroyed
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: a0310d6e04132a288a11410885acece94e4e2822
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found a blocking regression in the memory capability rename: the new ironclaw.memory.* IDs are not mapped back to the existing core/legacy memory tool names used by progressive disclosure and trace/scripted calls.
Findings
1. ❌ [MEDIUM] Memory tools no longer match the core tool-disclosure aliases
Location: crates/ironclaw_host_runtime/src/first_party_tools/memory.rs:30-33
Renaming the capability IDs from builtin.memory_write / builtin.memory_search to ironclaw.memory.write / ironclaw.memory.search needs a matching update in the tool-disclosure aliasing path. crates/ironclaw_reborn/src/tool_disclosure.rs still treats the core memory tools as memory_write, memory_read, and memory_search, and its matching logic only handles builtin.* suffixes or exact dotted/encoded IDs. As a result the advertised ironclaw__memory__write style names no longer match the core memory_write aliases, and existing scripted/recorded calls such as builtin.memory_write cannot resolve to the new capability. Update the disclosure matching/legacy alias tests to map ironclaw.memory.{write,read,search,tree} to the established memory_{write,read,search,tree} names before changing the IDs.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
| // as the `ironclaw.memory` extension (backed by the native provider by default, | ||
| // swappable via the document-store binding). The model-facing tool names derive | ||
| // from these ids (`.` -> `__`): `ironclaw__memory__{read,write,search,tree}`. | ||
| pub const MEMORY_SEARCH_CAPABILITY_ID: &str = "ironclaw.memory.search"; |
Contributor
There was a problem hiding this comment.
This ID rename also needs the progressive-disclosure legacy/core alias path updated. Today tool_disclosure.rs recognizes core memory tools as memory_write/memory_read/memory_search and has builtin-specific leniency, but it does not map ironclaw.memory.write or ironclaw__memory__write back to memory_write. That drops memory tools out of the expected core surface and breaks existing scripted/recorded calls like builtin.memory_write.
|
🚅 Deployed to the ironclaw-pr-5205 environment in ironclaw-ci-preview
|
Contributor
🗂️ Archived IronLoop Review: reviewerThis result is from an older PR head and is no longer the active review.
Archived summaryNo concrete blocking issues found in the memory provider binding/mem0 swap changes. The implementation keeps host-runtime provider-neutral, gates mem0 behind composition feature/config, and adds focused tests for binding policy, provider factory, mem0 request shaping, profile reads, and memory context admission. |
Contributor
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 13825bee2107b9457f8a3dfe045181a3e2a58a91
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete blocking issues found in the memory provider binding/mem0 swap changes. The implementation keeps host-runtime provider-neutral, gates mem0 behind composition feature/config, and adds focused tests for binding policy, provider factory, mem0 request shaping, profile reads, and memory context admission.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
ironloopai
Bot
dismissed
their stale review
July 6, 2026 15:29
Superseded by a later IronLoop approved review for this reviewer.
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Collaborator
Author
|
/canary |
Contributor
|
Started Reborn WebUI v2 live canary for |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.json (1)
2-5: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
items: {}still leaves array elements unconstrained.Same gap as previously flagged: any element type validates, so malformed tree entries pass schema validation. Not yet addressed in this snapshot.
🤖 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_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.json` around lines 2 - 5, The tree schema still leaves array elements unconstrained because the items definition in the memory tree JSON schema is empty. Update the schema for the tree output so the items shape is explicitly defined with the expected memory document/directory structure, using the tree schema file’s array definition to constrain each entry and prevent invalid tree elements from validating.crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.md (1)
1-3: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"profile facts" wording is still present.
This line still promises profile-fact recall via
read(path), though profile data is served throughMemoryService::profile_read, not the document-storereadsurface. This exact text was previously flagged and marked addressed, but is unchanged here.Proposed fix
-Use it to recall previously written notes, logs, or profile facts before acting. +Use it to recall previously written notes or logs before acting.🤖 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_host_runtime/assets/memory_native/prompts/memory-native/read.md` around lines 1 - 3, The read.md description for the document-store read surface still mentions “profile facts,” which incorrectly suggests that `read(path)` can retrieve profile data. Update the wording in the memory-native prompt to describe only persistent document reads for notes/logs/agent content, and remove any reference to profile-fact recall so it aligns with `MemoryService::profile_read` being the separate profile access path.crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json (2)
10-13: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
messagedescription still scoped to "cleared status" only.Previously flagged as mismatched against the mem0 write path (which sets
messageonWrittenstatus too); this was marked addressed, but the description text in this snapshot is unchanged. Please confirm whether the fix landed elsewhere or re-broaden this description.🤖 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_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json` around lines 10 - 13, The schema for document-write.output.v1 still narrows `message` to “cleared status” even though the mem0 write path also populates it for `Written` status. Update the `message` field description in the document write output schema to cover all statuses that can emit a human-readable note, and verify the corresponding writer logic (for example the mem0 write output handling) matches the broadened contract.
4-16: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
statusstill doesn't discriminate the payload.
{"status":"patched","path":"x"}and{"status":"cleared","path":"x","append":true}both still validate — properties aren't gated bystatusviaoneOf/if-then. Same issue as previously raised, still present.🤖 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_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json` around lines 4 - 16, The memory document write output schema still allows fields for all outcomes regardless of status, so update the document-write.output.v1.json schema to discriminate payloads by status. Use the schema definition for the status field and add conditional validation with oneOf or if/then/else so patched only accepts replacements and content_length, cleared only accepts message, and written only accepts append and content_length as appropriate. Keep the existing shape in sync with the status-driven cases in this schema.crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json (1)
4-9: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSchema still accepts
..traversal patterns.
pattern: "^[^/]"only rejects a leading/; strings likea/../../etc/passwdstill pass schema validation. The comment says the host filesystem rejects..separately, but the schema itself provides no defense-in-depth here — same gap as previously raised.🤖 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_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json` around lines 4 - 9, The document-read schema still allows path traversal via `..`, since the current path constraint only blocks a leading slash. Update the path validation in memory_native’s document-read schema to explicitly reject traversal segments like `..` in addition to absolute paths, using the existing `path` property definition so the schema itself enforces the restriction rather than relying only on host-side checks.crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.json (1)
10-17: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStill-open:
pathremains unconstrained in search results.Unlike the input schema (which now has tight validation), this output schema's
pathfield is still a bare string, so search results can surface absolute or../paths to the model. This was flagged previously and does not appear to have been addressed here.🛡️ Proposed schema hardening
- "path": { "type": "string" }, + "path": { + "type": "string", + "pattern": "^(?!/)(?!.*(?:^|/)\\.\\.(?:/|$))(?!.*\\\\)[A-Za-z0-9._/-]+$" + },🤖 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_host_runtime/assets/memory_native/schemas/memory/search.output.v1.json` around lines 10 - 17, The search output schema still leaves `path` as an unconstrained string, so tighten `memory/search.output.v1.json` by updating the `path` property in the search result schema to use the same path restrictions already applied in the input schema. Keep the fix scoped to the search output definition and ensure the `properties` block for `content`, `score`, `path`, and `is_hybrid_match` still validates results correctly while preventing absolute or parent-directory style paths from being emitted.crates/ironclaw_memory/src/service.rs (1)
357-370: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
MemoryServiceContextSnippetscope fields are still rawString, not newtypes.
tenant_id,user_id,agent_id,project_idremain plainString/Option<String>even though the doc comment says the host hashes them into the stablememory-snippet:*reference. A malformed or swapped scope component still serializes cleanly and silently changes snippet identity.As per coding guidelines, "Identifiers must use newtypes such as
CredentialName,ExtensionName,ThreadId, orUserId; do not useString,&str, oruuid::Uuidalone for identity values."🤖 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/src/service.rs` around lines 357 - 370, `MemoryServiceContextSnippet` still uses raw `String`/`Option<String>` for identity-bearing scope fields, so update the struct to use the appropriate newtypes instead of plain strings. Adjust `tenant_id`, `user_id`, `agent_id`, and `project_id` to the existing identity wrapper types used elsewhere in the service, and then fix any constructors, serializers, or call sites that build or consume `MemoryServiceContextSnippet` so they pass and preserve those typed values consistently. Use the `MemoryServiceContextSnippet` definition and its surrounding `service.rs` helpers as the main locations to update.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 `@crates/ironclaw_memory_mem0/src/url_check.rs`:
- Around line 33-35: Update the module docs in url_check.rs to match the actual
behavior of the URL rejections: the scheme-only path does not echo the parsed
URL, but the cloud-host and blocked-IP paths in the URL validation logic do
include the host/IP in Mem0Error::InvalidUrl.reason. Adjust the comment near the
URL checks to describe that only some rejection paths avoid echoing the raw URL,
and make sure the wording references the url_check validation flow and the
rejection cases covered by rejection_error_does_not_echo_the_configured_url and
rejects_aws_metadata_ip.
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 1147-1161: The local-dev mem0 app_id generation in the
workspace-binding logic uses DefaultHasher, which is not stable across
Rust/toolchain versions. Update the app_id derivation in the factory.rs
memory_provider_connection block to use a stable digest of the canonical root
path instead of DefaultHasher, while preserving the existing ws-* format and the
override behavior when app_id is already set.
---
Duplicate comments:
In
`@crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.md`:
- Around line 1-3: The read.md description for the document-store read surface
still mentions “profile facts,” which incorrectly suggests that `read(path)` can
retrieve profile data. Update the wording in the memory-native prompt to
describe only persistent document reads for notes/logs/agent content, and remove
any reference to profile-fact recall so it aligns with
`MemoryService::profile_read` being the separate profile access path.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json`:
- Around line 4-9: The document-read schema still allows path traversal via
`..`, since the current path constraint only blocks a leading slash. Update the
path validation in memory_native’s document-read schema to explicitly reject
traversal segments like `..` in addition to absolute paths, using the existing
`path` property definition so the schema itself enforces the restriction rather
than relying only on host-side checks.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json`:
- Around line 10-13: The schema for document-write.output.v1 still narrows
`message` to “cleared status” even though the mem0 write path also populates it
for `Written` status. Update the `message` field description in the document
write output schema to cover all statuses that can emit a human-readable note,
and verify the corresponding writer logic (for example the mem0 write output
handling) matches the broadened contract.
- Around line 4-16: The memory document write output schema still allows fields
for all outcomes regardless of status, so update the
document-write.output.v1.json schema to discriminate payloads by status. Use the
schema definition for the status field and add conditional validation with oneOf
or if/then/else so patched only accepts replacements and content_length, cleared
only accepts message, and written only accepts append and content_length as
appropriate. Keep the existing shape in sync with the status-driven cases in
this schema.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.json`:
- Around line 10-17: The search output schema still leaves `path` as an
unconstrained string, so tighten `memory/search.output.v1.json` by updating the
`path` property in the search result schema to use the same path restrictions
already applied in the input schema. Keep the fix scoped to the search output
definition and ensure the `properties` block for `content`, `score`, `path`, and
`is_hybrid_match` still validates results correctly while preventing absolute or
parent-directory style paths from being emitted.
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.json`:
- Around line 2-5: The tree schema still leaves array elements unconstrained
because the items definition in the memory tree JSON schema is empty. Update the
schema for the tree output so the items shape is explicitly defined with the
expected memory document/directory structure, using the tree schema file’s array
definition to constrain each entry and prevent invalid tree elements from
validating.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 357-370: `MemoryServiceContextSnippet` still uses raw
`String`/`Option<String>` for identity-bearing scope fields, so update the
struct to use the appropriate newtypes instead of plain strings. Adjust
`tenant_id`, `user_id`, `agent_id`, and `project_id` to the existing identity
wrapper types used elsewhere in the service, and then fix any constructors,
serializers, or call sites that build or consume `MemoryServiceContextSnippet`
so they pass and preserve those typed values consistently. Use the
`MemoryServiceContextSnippet` definition and its surrounding `service.rs`
helpers as the main locations to update.
🪄 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: 25765b7e-908c-4bfa-ae55-323534f343bf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (72)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_capabilities/tests/capability_profile_conformance_contract.rscrates/ironclaw_extensions/tests/manifest_v2_contract.rscrates/ironclaw_host_api/src/host_port.rscrates/ironclaw_host_runtime/assets/memory_native/manifest.tomlcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.mdcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/search.mdcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/tree.mdcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/write.mdcrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.jsoncrates/ironclaw_host_runtime/src/extension_contracts.rscrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/memory_binding.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/memory_native_extension.rscrates/ironclaw_host_runtime/src/memory_profiles.rscrates/ironclaw_host_runtime/src/memory_provider.rscrates/ironclaw_host_runtime/src/surface.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/memory_capability_profiles.rscrates/ironclaw_host_runtime/tests/memory_native_manifest.rscrates/ironclaw_host_runtime/tests/memory_native_schema_validation.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_host_runtime/tests/user_profile_roundtrip.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_mem0/Cargo.tomlcrates/ironclaw_memory_mem0/src/config.rscrates/ironclaw_memory_mem0/src/error.rscrates/ironclaw_memory_mem0/src/lib.rscrates/ironclaw_memory_mem0/src/service.rscrates/ironclaw_memory_mem0/src/transport.rscrates/ironclaw_memory_mem0/src/url_check.rscrates/ironclaw_memory_mem0/tests/live_local_mem0.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_reborn/src/tool_disclosure.rscrates/ironclaw_reborn_cli/Cargo.tomlcrates/ironclaw_reborn_cli/src/runtime/mod.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/local_dev_capability_policy.tomlcrates/ironclaw_reborn_composition/src/memory_binding.rscrates/ironclaw_reborn_composition/src/memory_provider_factory.rscrates/ironclaw_reborn_composition/src/projection/display_preview.rscrates/ironclaw_reborn_composition/src/projection/tests/display_preview.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/src/test_support/user_profile.rscrates/ironclaw_reborn_composition/tests/memory_mem0_swap.rscrates/ironclaw_reborn_config/src/config_file.rscrates/ironclaw_reborn_config/src/lib.rsdeny.tomldocs/adr/0001-capability-manifest-v2-hard-cutover.mddocs/adr/0002-native-memory-uses-host-storage-ports.mddocs/reborn/contracts/memory-profiles.mdscripts/ci/package-feature-flags.shtests/reborn_trace_first_party_tool_coverage.rs
Comment on lines
+33
to
+35
| // None of these rejections carry the raw URL: only a redacted `reason` survives | ||
| // into the error (see `Mem0Error::InvalidUrl`), so a misconfigured host or a | ||
| // query-string token cannot leak into host logs. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Module doc overstates the "no host leak" guarantee.
Lines 33-35 claim none of the rejections carry the host/URL into the error, but the cloud-host (line 69) and blocked-IP (line 79) rejections both interpolate {host} directly into reason. The redaction test (rejection_error_does_not_echo_the_configured_url) only covers the scheme-rejection path where host is never parsed, so it doesn't actually validate the doc's claim — and rejects_aws_metadata_ip even asserts the IP is present in the error text. Not exploitable today (hosts/IPs aren't secrets), but the comment should describe actual behavior so callers don't assume stronger redaction than what's enforced.
Suggested doc fix
//! Returns `Err(Mem0Error::InvalidUrl { .. })` on parse failure, non-http(s)
//! scheme, embedded credentials, missing host, the hosted mem0 cloud host, or an
//! `AlwaysBlocked` literal IP host.- // None of these rejections carry the raw URL: only a redacted `reason` survives
- // into the error (see `Mem0Error::InvalidUrl`), so a misconfigured host or a
- // query-string token cannot leak into host logs.
+ // Scheme/credential/parse rejections never surface the URL. The cloud-host and
+ // blocked-IP rejections below intentionally include the offending host/IP in
+ // `reason` for debuggability; only credentials and query-string tokens are
+ // guaranteed redacted.Also applies to: 62-72, 78-81
🤖 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_mem0/src/url_check.rs` around lines 33 - 35, Update
the module docs in url_check.rs to match the actual behavior of the URL
rejections: the scheme-only path does not echo the parsed URL, but the
cloud-host and blocked-IP paths in the URL validation logic do include the
host/IP in Mem0Error::InvalidUrl.reason. Adjust the comment near the URL checks
to describe that only some rejection paths avoid echoing the raw URL, and make
sure the wording references the url_check validation flow and the rejection
cases covered by rejection_error_does_not_echo_the_configured_url and
rejects_aws_metadata_ip.
Comment on lines
+1147
to
+1161
| // LOCAL-DEV ONLY: bound mem0 memory to this workspace so memories from one | ||
| // local-dev root never leak into another, even when both point at the same | ||
| // mem0 server. A stable hash of the canonical root path is used as the mem0 | ||
| // `app_id`; this mirrors how native memory is already isolated (its | ||
| // filesystem store lives under `local_dev_root`). An explicitly-configured | ||
| // `app_id` always wins (operator override takes precedence). | ||
| // | ||
| // Production paths (`build_*_production` / `RebornProductionBuildContext`) | ||
| // are NOT touched here: production keeps `app_id` from config (default None | ||
| // → pure user/tenant scope, persistent across restarts for the same tenant). | ||
| if memory_provider_connection.app_id.is_none() { | ||
| use std::hash::{DefaultHasher, Hash, Hasher}; | ||
| let mut hasher = DefaultHasher::new(); | ||
| root.hash(&mut hasher); | ||
| memory_provider_connection.app_id = Some(format!("ws-{:016x}", hasher.finish())); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no persistent namespace/id derivation still relies on DefaultHasher.
rg -n "DefaultHasher|stable hash|ws-" crates/ironclaw_reborn_composition/src/factory.rsRepository: nearai/ironclaw
Length of output: 445
🌐 Web query:
Rust std::hash::DefaultHasher stability contract persistent identifier documentation
💡 Result:
The Rust standard library explicitly does not provide a stability guarantee for the output of std::hash::DefaultHasher [1][2]. According to the official Rust documentation, the internal algorithm used by DefaultHasher is not specified, and its resulting hashes should not be relied upon across different releases of the compiler [1][2]. Furthermore, the data fed by standard library types into a Hasher is not considered stable between compiler versions [3][4]. Consequently, DefaultHasher output is not guaranteed to be portable or consistent across compiler versions or platforms [3][4]. The documentation strongly advises against using DefaultHasher or the Hash trait for tasks that require stability or portability, such as hashing data for long-term storage or serialization formats intended to work across different environments [3][4]. Developers are explicitly warned that tests should not probe hard-coded hash values, but should instead check consistency via the Eq trait [3][4]. For stable, cross-version, or cross-platform hashing, you should avoid relying on the standard library's Hash and Hasher traits and instead use specific, third-party hashing algorithms that provide explicit stability guarantees [3][5].
Citations:
- 1: https://doc.rust-lang.org/std/hash/struct.DefaultHasher.html
- 2: https://doc.rust-lang.org/stable/std/hash/struct.DefaultHasher.html
- 3: https://doc.rust-lang.org/stable/std/hash/trait.Hash.html
- 4: https://dev-doc.rust-lang.org/beta/core/hash/trait.Hash.html
- 5:
Hashportability between compiler versions is potentially misleading rust-lang/rust#144540
Use a stable digest for the local-dev mem0 app_id. std::hash::DefaultHasher has no cross-version stability guarantee, so ws-* can change after a Rust/toolchain update and strand existing workspace memories.
🤖 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 1147 - 1161,
The local-dev mem0 app_id generation in the workspace-binding logic uses
DefaultHasher, which is not stable across Rust/toolchain versions. Update the
app_id derivation in the factory.rs memory_provider_connection block to use a
stable digest of the canonical root path instead of DefaultHasher, while
preserving the existing ws-* format and the override behavior when app_id is
already set.
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5205
July 6, 2026 16:05
Destroyed
Contributor
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: 0f0a350b0fe4c67a4b6bbff4be1c33db4edcd079
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found a blocking compile error in the local-dev memory resolver wiring.
Findings
1. ❌ [HIGH] Fix invalid DefaultHasher import
Location: crates/ironclaw_reborn_composition/src/factory.rs:1158
DefaultHasher is not exported from std::hash; it lives at std::collections::hash_map::DefaultHasher. This import makes the composition crate fail to compile anywhere build_local_runtime is built. Split the import, e.g. use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher};.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
| // are NOT touched here: production keeps `app_id` from config (default None | ||
| // → pure user/tenant scope, persistent across restarts for the same tenant). | ||
| if memory_provider_connection.app_id.is_none() { | ||
| use std::hash::{DefaultHasher, Hash, Hasher}; |
Contributor
There was a problem hiding this comment.
DefaultHasher is not in std::hash; this does not compile. Import it from std::collections::hash_map::DefaultHasher and keep Hash/Hasher from std::hash.
Contributor
Reborn integration-tier coverageLine coverage (Reborn crates): 28.51% — 49350 / 173073 lines Per-crate breakdown (62 crates, lowest-covered first)
This signal is informational: coverage never gates the PR — not the percentage, not the per-crate holes, not the 0-coverage callout. Exemptions (0 file(s) excluded from the accounting above)No exemptions configured. |
Collaborator
Author
|
/canary |
Contributor
|
Started Reborn WebUI v2 live canary for |
BenKurrek
added a commit
that referenced
this pull request
Jul 20, 2026
…ter-turn record (mem0 flow) (#5327) * feat(memory): host-managed memory lifecycle — two-lane retrieval + after-turn record (mem0 flow) Implements the mem0 host-managed memory flow on top of #5205, with the surface area confined to the native memory provider and run-level orchestration. Retrieval (once per run): - The loop fetches long-term (user-general) and short-term (per-thread, `threads/<thread_id>/`) memory once at the first prompt build of a run and injects both into the prompt's "memory" section, replacing the dead `memory_snippets: Vec::new()` in loop_support. A per-run OnceCell caches the fetch; subsequent model steps reuse it. - Native `retrieve_context` scopes by `invocation.scope.thread_id`: Some(T) → only that thread's `threads/<T>/` subtree (short-term); None → the user's general memory, excluding `threads/*` (long-term). The lanes are disjoint, so the host concatenates them (short-term first) under the existing 4 KiB admission budget. - Graceful degradation throughout: a memory failure degrades the lane to empty and never breaks a turn. Recording (after each turn): - New low-level `MemoryService::record_interaction(invocation, { messages, run_id, metadata })` — the mem0 `add` data shape (`user_id`/`agent_id`/ `thread_id` ride the invocation scope). A default no-op trait impl lets each provider opt in: the host passes the DATA and the provider decides what to do with it (store verbatim, run LLM extraction, or nothing). Implements the reserved `memory.interaction.record.v1` vocabulary. - The native provider stores the full turn history under `threads/<thread_id>/`. - A host `AfterTurnMemoryRecorder` fires at the run-end seam (`turn_run_executor::apply_exit`, gated on `Completed`), reads the exchange from the thread transcript with the owner-rewritten scope, and hands it down. Post-terminal and best-effort: every error is `debug!`-logged and never fails the already-completed run. Reads and writes resolve on the local-dev runtime path; the production graph wires `None` (deferred, issue #5013 — the same optionality as `user_profile_source`). Tested at unit + caller level across ironclaw_memory{,_native}, host_runtime, loop_support, turns, and reborn (two-lane fetch, prompt rendering, once-per-run cache, native record→retrieve, and a full-turn record through the executor). A full-composition e2e (record in one run → surface in a later run's model request) is called out as a follow-up. Design: docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address review — full transcript (H1), run-vs-session, idempotency, build break Addresses the adversarial audit + the mem0 data-parity audit + CodeRabbit on #5327. - H1 + parity (transcript): `build_exchange` → `build_transcript` now captures the FULL ordered run transcript — every user/assistant/tool message of the turn, in sequence order, every finalized assistant including the FINAL answer (fixes recording the first/intermediate assistant on multi-step runs), each tagged with its actor `name`. This is the data mem0's `add` receives. - Run-vs-session (parity): rename `MemoryServiceRecordRequest.run_id` → `turn_run_id` (per-turn provenance). mem0's session id maps to `scope.thread_id` (the conversation), NOT this field — documented on the contract + recorder so a mem0 provider can't mis-map (which would write under the turn id but read under the session id → silent short-term-recall miss). - Idempotency (CodeRabbit): native `record_interaction` writes the transcript to a per-run file `threads/<thread_id>/<turn_run_id>.md` with `append: false` (overwrite), so a scheduler re-run of an already-Completed run can't duplicate the exchange or grow an unbounded `log.md`. - Parity: add `MemoryInteractionMessage.name` (mem0 message name → per-memory `actor_id`); populate `metadata` with `{turn_run_id, correlation_id}` provenance. - Build break (CodeRabbit, critical): add the missing `after_turn_memory_writer` field to the root crate's `tests/support/reborn/harness.rs` (was `E0063`). The gate now compiles the root crate's reborn tests. - Audit M1: the per-run memory `OnceCell` no longer freezes to empty when the first prompt build has no user message — it seeds only once a real request exists. - Audit L3: `// arch-exempt: optional_arc` annotations on the new optional fields; corrected the misleading "mirrors user_profile_source" comments. - Docs: long-term lane is full-tuple `(tenant,user,agent,project)` scoped; the `threads/` reservation is advisory (L1); fetch-once-per-run has no mid-run invalidation in v1 (Q6). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address re-review — lane starvation, threads/ write-reject, no transcript trim, render ordering, fetch-once-per-run CodeRabbit: - retrieve_context: over-fetch BEFORE the short-term thread-lane filter, then truncate, so general hits in the global top-N can no longer starve the thread-scoped lane (TDD). - write: reject the reserved `threads/` namespace (fail loud). record_interaction still writes there via a new private write_reserved_document bypass (TDD). Un-defers audit L1. - build_transcript: stop trimming message content — filter blank-only rows but pass content through verbatim ("LLM data is never deleted") (TDD). - InstructionBundleBuilder::build: preserve the host's short-term-first memory order; drop the by-ref re-sort that scrambled lane priority at the render boundary (TDD; updates the now-obsolete model_content-tiebreak test). - loop_driver_host: caller-level test through build_default_planned_runtime that the after_turn_memory_writer is plumbed into the executor (not just the builder shortcut). - threadless record_interaction test: supply a real turn_run_id to isolate the no-thread branch; soften the after-turn writer contract comments (runtime.rs + after_turn_memory.rs) to match the full-transcript behavior. Gemini: - load_memory_snippets_once: get_or_init + cache empty on failure = true fetch-once-per-run, no retry-storm on a slow/down memory service (M1 early return preserved). - memory_context: share one correlation_id across both retrieval lanes. Also: strengthen the aggregate-budget test with a non-empty assertion (so the all-short-term check isn't vacuous), and align the design doc with shipped behavior (no mid-run invalidation; full transcript; threads/ enforced). cargo fmt + clippy clean (zero warnings); per-crate tests green: memory_native, host_runtime, turns, loop_support, reborn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address re-review round 3 — non-blank query, reserved-write guard, bounded recorder, silent-ok, test race CodeRabbit (round 3): - latest_user_message_text: return the latest NON-BLANK user message so a blank trailing user turn doesn't drop proactive memory for the run (+ unit test). - write_reserved_document: enforce the `threads/` bypass — reject any non-threads resolved path so the public `write` guard can't be circumvented via this helper. - after_turn_memory: annotate the two intentional post-terminal fallbacks with `// silent-ok:` per the fail-loud convention. - turn_run_executor: bound the inline after-turn recorder await with a 30s timeout so a slow/hung provider can't occupy the scheduler worker. - loop_driver_host: move the sibling test's scheduler shutdown after the memory read/asserts so it can't race the recorder. - reborn_composition: soften the after-turn `[user, assistant]` contract comment to match full-transcript behavior (same fix as runtime.rs). fmt + clippy clean; tests green (loop_support 336, memory_native 20, reborn 258 + loop_driver_host 109). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address re-review round 4 — fetch memory lanes concurrently retrieve_context awaited the short-term and long-term lanes sequentially; they are independent (share only the Copy correlation id), so fetch them with tokio::join! to cut added run-start prompt-path latency, keeping short-term-first concatenation. (CodeRabbit, trivial.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(memory): self-contained read_long_term/read_thread on MemoryService Fold the prompt-context lane orchestration into the memory service (review feedback): the host no longer knows about "lanes." Two new provider-agnostic default trait methods own both the lane scoping AND the per-snippet safety: - read_long_term: clears the thread sub-scope (general memory), then sanitizes each candidate (scope-check + control-strip + size-cap + untrusted envelope). - read_thread: keeps the thread sub-scope (this conversation), same safety. Providers implement only the raw retrieve_context; they inherit the safe lane reads, so no provider can return unsafe memory context. ironclaw_memory gains a leaf dependency on ironclaw_prompt_envelope (no cycle); the sanitize helpers + scope check move there with their unit tests. The host ProductionMemoryPromptContextService collapses to two scoped reads + a trivial map to LoopContextSnippet (net -322 lines). The map keeps only the two loop-layer steps that can't move down without a dependency cycle: the model-visible reference and the loop's prompt-content denylist drop-filter (LoopSafeSummary), a prompt-layer policy applied to all model context. Deleted: the MemoryLane enum, retrieve_lane, admit_memory_context_snippet, ExpectedSnippetScope, and the host-side sanitizer. Behavior change: prompt order is now long-term then short-term (this conversation nearest the current message, per the mem0 on_run_start shape), which also flips which lane wins under the shared 4 KiB budget (was short-term-first). read_profile is unchanged: profile_read already returns the raw doc and the host parses it into the loop-shaped UserProfileContext — the correct provider-neutral split, not the lane confusion this refactor targets. fmt + clippy clean; tests green across ironclaw_memory, ironclaw_memory_native, ironclaw_host_runtime; consumers (mem0 / reborn / reborn_composition) compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address host lifecycle review feedback --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings PR #5205 (memory as a userland extension, #3537) up to date with main (277 commits of drift). Notable conflict reconciliations: - user_profile_source.rs: combined main's read-through cache + single-flight with #5205's provider-neutral MemoryService::profile_read facade. - first_party_tools/mod.rs + composition/factory.rs: threaded main's now-required `active_run_lookup` trigger arg through #5205's `*_and_memory_resolver` builder chain (auto-merge left it inconsistent). - memory_native/service.rs: kept #5205's host-owns-sanitization architecture (provider returns raw snippet components). - Applied main renames into #5205 code: LocalHostProcessPort->HostProcessPort, LocalDevRootFilesystem->CompositeRootFilesystem, LocalFilesystem->DiskFilesystem; composition mod path profile->root::profile. - Followed main's removal of the `root-llm-provider` feature; kept #5205's independent `memory-mem0` feature; deduped a duplicated `ironclaw_memory` dep. - reborn_config: added Serialize to Memory{Section,ProfileBinding,AdminOverride} (main made RebornConfigFile serializable); config `read` expander + cli mem0 env wiring updated for the new `memory` section and main's Option-returning `optional_nonempty_env`. - Architecture test: took main's comprehensive boundary canvas + re-added #5205's mem0 provider-neutrality test; declared the mem0 crate layer; allowlisted memory_binding.rs deployment-profile branching. - Regenerated the 3 golden payload snapshots + composition pub-use snapshot. Verified: fmt, clippy -D warnings (host_runtime, composition, architecture, reborn_cli), full workspace check, cargo-deny, and targeted tests (architecture, memory unit tests, wiring_parity, group_memory, tool_call, golden_payload, first-party coverage). Pre-existing Docker-only sandbox_process tests still fail locally without Docker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
contributor: core
BenKurrek
added a commit
that referenced
this pull request
Jul 20, 2026
…and-extension Brings PR #6345 (= #5205 + the host-managed memory lifecycle commit #5327) up to date with main by merging the freshly-updated #5205 branch. rerere auto-replayed all 24 shared #5205 conflict resolutions; this commit resolves the incremental lifecycle-commit conflicts against main's refactored runner: - Crate renames applied: ironclaw_reborn -> ironclaw_runner, ironclaw_loop_support -> ironclaw_loop_host (the lifecycle's new files after_turn_memory.rs / memory_context.rs relocated into the renamed crates). - loop_host prompt builder: placed the lifecycle's proactive two-lane memory fetch (load_memory_snippets_once) after main's `tokio::try_join!` context restructure, before `context.messages` is consumed; kept main's `resolution` re-export + trace_loop_host_latency_ok. - turn_run_executor / runtime: threaded BOTH main's required `active_run_lookup` + `gate_record_store` and the lifecycle's `after_turn_memory_recorder` through the executor construction. - runner Cargo.toml: main's non-optional ironclaw_llm + loop_host rename + processes test-support, plus the lifecycle's ironclaw_memory / ironclaw_memory_native deps. - loop_driver_host test: ported the 2 lifecycle memory tests onto main's APIs — the §3 subagent_gate_store -> await_edge {writer,settler,evidence} split (build_test_await_edge_trio), 4-arg RebornTurnRunExecutor::new, ThreadMessageRecord created_at/updated_at, SubmitTurnRequest.requested_model. - wiring_parity / planned_runtime_parts_shape: EXPECTED_PRODUCTION_SHAPE unions gate_record_store + memory_context_service + after_turn_memory_writer (16 Option fields). - architecture: allowed ironclaw_memory -> ironclaw_prompt_envelope (the lifecycle's prompt-safe memory-context wrapping, #5327). Verified: fmt, clippy -D warnings (runner, loop_host, composition), full workspace check, and targeted tests (architecture, runner memory lifecycle, wiring_parity, group_memory, golden_payload, first-party coverage). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Superseded by #6345, which contains every commit from this branch plus the host-managed memory lifecycle (#5327) and is current with |
BenKurrek
added a commit
that referenced
this pull request
Jul 24, 2026
…ecycle — implements #3537 (#6345) * test(architecture): enforce reborn dependency boundaries as a workspace-wide allowlist PR #5163 made only the memory rules allowlist-based; the other ~30 `BoundaryRule` entries stayed blocklists that under-enforce — they forbid today's offenders but would silently admit a future internal dep (e.g. `ironclaw_turns`, `ironclaw_product_workflow`, `ironclaw_reborn`). Convert the whole harness to an allowlist: - `BoundaryRule` now carries `allowed: Vec<&'static str>` instead of `forbidden`. The runner computes `forbidden = workspace_ironclaw_crates() - allowed - crate_name`, so any unlisted internal dependency now fails the boundary test. - Each crate's `allowed` set is its actual normal `ironclaw_*` dependencies, matching the dependency guardrail documented in its CLAUDE.md/AGENTS.md. - The three previously-inline allowlist rules (`ironclaw_host_api`, `ironclaw_memory`, `ironclaw_memory_native`) are folded into `boundary_rules()` so the test body is a single uniform loop. Behavior-preserving on the current (correct) dependency graph; the win is forward enforcement. Addresses serrrfirat's deferred thread on #5163 (discussion_r3468163078). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(memory): make the host the sole constructor of admitted memory context Before this change the native provider sanitized, wrapped, and hashed each memory snippet, and the host only *asserted* the `Untrusted memory content:` prefix in `admit_memory_context_snippet`. A future untrusted provider could pre-attach that prefix (or pre-shape the snippet) and slip text past the host's prompt-safety wrapper. Move all model-visible shaping into the host so the provider can never bypass prompt safety: - `MemoryServiceContextSnippet` now carries RAW snippet text plus the resolved scope/path components (`tenant_id`, `user_id`, `agent_id`, `project_id`, `relative_path`) — no `snippet_ref`/`safe_summary`/`model_content`. - The native `retrieve_context` ranks and scope-filters candidates, then returns them raw; it no longer sanitizes, truncates, hashes, or budgets. Removed the native `sanitize_snippet_text`, `truncate_to_char_boundary`, `validate_loop_safe_summary`, `memory_snippet_display_ref`, `feed_hash`, `collect_context_snippets`, the FNV/budget consts, and the prompt-envelope dep. - The host `memory_context.rs` builds the `memory-snippet:*` reference via the canonical `ironclaw_turns::run_profile::memory_snippet_display_ref`, sanitizes + wraps the raw text (`sanitize_snippet_text` relocated here), validates through the loop's own `LoopSafeSummary` gate (collapsing the native denylist copy into one source of truth), and enforces the per-snippet (512B) + aggregate (4 KiB) budgets in the admission loop with the same break semantics the native `collect_context_snippets` used. Behavior-preserving for the native provider: model-visible output is byte-for-byte identical (same wrapping, same FNV trailing-separator `memory-snippet:cb96ed00b13e6ae4` golden ref, same caps/ordering). New coverage proves a provider that returns text merely starting with the untrusted prefix is STILL re-sanitized + re-wrapped by the host (`adapter_re_sanitizes_provider_supplied_untrusted_prefix`, `sanitize_re_wraps_text_already_carrying_untrusted_prefix`), plus the legacy ref golden lock and the host-owned aggregate-budget test. Addresses serrrfirat's deferred security thread on #5163 (discussion_r3468163070; ref-stability discussion_r3466587649). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(memory): route user-profile reads through the MemoryService facade Profile READS built the native repository/backend directly in `user_profile_source.rs` and duplicated the scope/path decision (`profile_scope_and_path` + `PROFILE_DOCUMENT_PATH`) that `NativeMemoryService` already owns for WRITES (`profile_set`). That coupled profile reads to the concrete native provider and left provider selection unable to swap reads with the rest of the memory facade. Add a provider-neutral profile read to the contract and route the host read through it: - New `MemoryService::profile_read(invocation) -> MemoryServiceProfileReadResponse` (raw document bytes) on the `ironclaw_memory` trait, with a native implementation that reuses the SAME `profile_scope_and_path` as `profile_set` — so the scope/path decision lives in exactly one place per provider. - `MemoryBackedUserProfileSource` now holds `Arc<dyn MemoryService>` and reads via `profile_read`; the host keeps only the parse + 64 KiB size-cap + validation. Deleted the duplicate host `profile_scope_and_path` / `PROFILE_DOCUMENT_PATH` and their re-export. - Production wiring uses the new `MemoryBackedUserProfileSource::from_filesystem` factory (host owns the native-provider choice, matching the memory capability); the composition layer keeps passing the workspace filesystem. Behavior-preserving: the unit tests assert identical parse/validation outcomes (now through a stub `MemoryService`), and the end-to-end `user_profile_roundtrip` test still proves the agent-scoped write → user-scoped read round trip, now through `MemoryService::profile_read`. Addresses serrrfirat's deferred thread on #5163 (discussion_r3466587663). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(memory): document the two context-path deltas from the regression audit Both deltas live in the (host-driven) context path and are intentional; this commit records why so a future reader doesn't "fix" them back toward origin: - Native `retrieve_context` uses `.with_vector(false)` while origin's prompt-context search left `vector=true`. `false` is correct for the FTS-only native backend (no embeddings wired; a vector request fails closed) and matches the native `search` method. Documented inline. - Host `map_memory_service_error` maps a failed memory-scope build to `InvalidInvocation` (via the provider's `Input` kind) where origin used `Internal`. The arm is unreachable in practice — the host validates the context scope before calling `retrieve_context` — and `InvalidInvocation` fails closed on the same axis as query validation. Documented on the mapper. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(memory): host memory profile catalog, host ports, native v2 manifest + binding policy (#3537) Land the host-runtime side of the remaining #3537 milestones: - M1: author the three memory CapabilityProfileContracts (context_retrieval, interaction_log, document_store) as host-defined code in `memory_profiles`, with repo conformance tests driving the real catalog through the `ironclaw_capabilities` harness. - M2: register `host.storage.sql_transaction.first_party` + `host.events.audit` (new `ironclaw_host_api` constants) in `default_host_port_catalog()`. - M3: bundle the `ironclaw.memory.native` v2 Extension Manifest (HostBundled, first_party runtime) under `assets/memory_native/`, parsed/backed from host_runtime so the manifest's `service` must match the registered native provider identity ("TOML alone is not authority"). Conformance + schema validation tests over the real bundled schemas. - M4 (host side): fail-closed `MemoryBindingPolicy` (profile_id -> provider, default-native, production rejects disabled/unverified-third-party absent an (extension_id, profile_id, deployment_profile) override). The memory-tools dispatch site now consults the binding instead of hardwiring `NativeMemoryService::from_filesystem`; non-native bindings fail closed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(memory): add [memory] profile-binding config section (#3537) Add the `[memory]` section to `RebornConfigFile` with `profile_bindings` (profile_id -> extension_id) and `admin_overrides` (scoped to (extension_id, profile_id, deployment_profile)). Validation is structural + deployment-agnostic (non-empty fields, valid override deployment_profile or `*`); profile-id validity and fail-closed production policy are owned by the host-runtime binding resolver, which holds the profile catalog. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(memory): wire memory binding through composition + CLI startup (#3537) Resolve the memory binding policy from the `[memory]` config section + the deployment profile at startup (fail-closed: production rejects memory.disabled / unverified third-party bindings without an override), and thread the resolved document-store binding to the builtin first-party handler registry on both the local-dev and production composition paths. The CLI resolves the policy in `build_services_input_with_options` and attaches it to `RebornBuildInput`; active third-party overrides are logged (redacted) at `debug!`. Replaces the hardwired native provider selection at the dispatch site with a config-driven, profile-bound resolution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(memory): add manifest-v2 + host-storage-port ADRs; update memory-profiles status (#3537) Add the two ADRs the issue references (0001 Extension Manifest v2 hard cutover, 0002 native memory uses host storage ports) and move memory-profiles.md from "draft zero-behavior" to Active, with an Implemented/Deferred split. Documents the gated remainder explicitly: the reborn_memory_* dual-backend SQL tables + concrete storage-port adapter + scoped HostPortView into the handler (boundary: composition crates cannot depend on the root ironclaw crate where the SQL backends live), and the default flip (blocked on /memory data + API compatibility tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address review on the #3537 memory binding (scope guard, profile binding, fail-loud) Apply the substantive + cheap review findings from the bot review pass: - Scope-equality guard (security): the provider-neutral snippet admission path (`memory_context::admit_memory_context_snippet`) now drops any snippet whose tenant/user/agent/project scope does not match the request scope before it is hashed or admitted. Native filters earlier, but a pluggable/third-party provider must not inject cross-scope content. Adds drop/keep tests. - Profile reads honor the binding: the local-dev user-profile source now builds the native-backed reader only when the document-store profile is bound to native, degrading to empty otherwise — so profile reads stay consistent with the memory tools instead of silently staying native. The resolved binding is carried on the local-dev store-graph input / local-runtime services. - Fail-loud: `document_store_binding` returns `Result` and surfaces a missing document-store binding instead of silently falling back to native. - Char-safe redaction: `MemoryActiveOverride::redacted_summary` truncates by characters, not bytes (the byte slice could not panic — id is ASCII — but the char form follows the repo rule and is encoding-robust). - More schema coverage: valid/invalid instance fixtures for document-read, document-write, and interaction-record (previously only context-retrieve). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(memory): single MemoryServiceResolver for every memory consumer (#3537) Collapse the per-call-site memory provider construction into one resolver. Before, the memory tools and the user-profile reader each called `NativeMemoryService::from_filesystem` and re-checked the binding inline, so the "which provider, and is it permitted?" decision was duplicated. `MemoryServiceResolver` (host_runtime::memory_provider) is now that decision in one place: given a profile + per-invocation inputs (filesystem + optional prompt-write-safety sink) it resolves the bound provider or returns None (fail-closed) for disabled / unimplemented-third-party bindings. It wraps `Option<MemoryBindingPolicy>` (None = native default) so it is Default and the tool structs that hold it need no fallible constructor. - Memory tools (MemoryCapabilityState) hold a resolver and build their service through it; they no longer reference NativeMemoryService or MemoryProviderBinding. - The local-dev user-profile source builds through the same resolver (native → MemoryBackedUserProfileSource, disabled/third-party → EmptyUserProfileSource). - The context retriever already takes an injected service and draws from the resolver once production-wired. - Composition threads one `MemoryServiceResolver` (built once per runtime from the resolved policy) instead of a bare `MemoryProviderBinding`; the `document_store_binding` helper is removed. Behavior-preserving for the native default; verified by an adversarial review (completeness / fail-closed / behavior-preservation / dead-code) plus the existing memory + user_profile_roundtrip suites. fmt + clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(memory): collapse memory to a single always-on ironclaw.memory.native package Collapse the two capability declarations of the one filesystem-backed memory provider into one. The model-facing memory tools (read/write/search/tree) now belong to a dedicated `ironclaw.memory.native` first-party package on the same always-on lane as `builtin` (registered directly into the builtin extension registry, not the catalog/lifecycle extension lane), replacing the anonymous `builtin.memory_*` capabilities. - read/write `implements` the `memory.document_store.v1` profile; search/tree are native conveniences that implement no profile. - Input schemas are served inline by `resolve_native_memory_input_schema_ref` via a provider-keyed `surface.rs` branch — no asset materialization, mirroring the builtin package. - The package is trusted (per-turn `provider_trust` insert + a first-party `AdminEntry`) and granted the /memory mount via the local-dev capability policy, exactly as the builtin memory tools were. Provider-swapping stays on the document-store profile binding. Behavior, I/O, data, and on-by-default availability are unchanged; only the capability identity and the derived model tool names change (builtin__memory_* -> ironclaw__memory__native__*). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(memory): load native memory from the bundled v2 TOML manifest (honor #3537) Path 1 code-constructed the native package in Rust; #3537 explicitly wants a bundled v2 Extension Manifest. This reworks native memory to parse the bundled `assets/memory_native/manifest.toml` and register the resulting package on the SAME always-on first-party lane (not the catalog/lifecycle lane), so it stays unconditionally available with no install/enable step — a v2 manifest on the always-on lane. - The manifest is reshaped from the dormant host_internal/SQL form into four model-visible memory tools: `read`/`write` implement `memory.document_store.v1` (their schema refs match the profile op refs); `search`/`tree` are native conveniences. No required host ports (filesystem-backed); the SQL/audit ports stay catalogued vocabulary for the deferred SQL milestone. - `memory_native_extension::native_memory_first_party_package()` parses the TOML (via `ExtensionManifestRecord`) into an `ExtensionPackage`; the composition registry insertion + trust + /memory grant from the prior commit are reused unchanged (same native capability ids). - Input schemas are served inline on the always-on lane via `include_str!` of the bundled asset files (the single source of truth) — no materialization. Prompt docs are added (required for model visibility) and bundled likewise. - Conformance tests updated to the lean scope: native satisfies `memory.document_store.v1`; the context_retrieval/interaction_log profiles remain defined for the deferred host-managed flow with no live implementer. `NATIVE_MEMORY_FIRST_PARTY_PROVIDER` now aliases the canonical `NATIVE_MEMORY_EXTENSION_ID` (single identity source). Behavior, I/O, data, and on-by-default availability remain unchanged from the prior commit; this changes the manifest authoring form (code -> bundled v2 TOML). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(memory): document the live always-on native v2 memory surface Update memory-profiles.md and ADR 0002 to reflect that ironclaw.memory.native is now live: its bundled v2 TOML manifest is parsed and registered on the always-on first-party lane, implementing memory.document_store.v1 via model-facing read/write tools (search/tree are native conveniences). The live provider is filesystem-backed and declares no host ports; the SQL/audit ports stay catalogued for the deferred SQL-backed milestone. The context_retrieval/interaction_log profiles remain defined with no live implementer (deferred host-managed flow). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): keep display-preview summaries working for native memory ids The projection display-preview summarizer keys on the `memory_<op>` short names via `capability_matches`, which matched `builtin.memory_<op>` by suffix. The native tools are now `ironclaw.memory.native.<op>` (suffix `.<op>`, not `.memory_<op>`), so memory input summaries — including write-content secret redaction — would have silently stopped rendering in production. Teach `capability_matches` the new id shape and update the projection test fixtures to the native ids so they exercise it. Also rename the now-misnamed `builtin_memory_search_dispatches_*` host_runtime test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): complete native-package test + schema updates for host_runtime The native-memory consolidation (ffbd298d6, dc2fbb53b) moved the memory_* capabilities out of the builtin package into the always-on `ironclaw.memory.native` package and reshaped the bundled input schemas to the four live document-store tools, but the host_runtime test suite + two schemas were left asserting the old shape — leaving `Test ironclaw_host_runtime` red on 18 memory tests. - first_party_builtin_tools.rs: resolve the memory capabilities from the native package (register it in the test registry + add its first-party trust entry) and drop the memory ids from `all_builtin_capability_ids`. - memory_native_schema_validation.rs: validate the four live tool schemas (read/write/search/tree); the removed context-retrieve / interaction-record schemas belong to the deferred host-managed flow and are no longer bundled. - search.input.v1.json: accept the `q`/`text`/`pattern` aliases that `MemoryServiceSearchRequest::from_tool_input` already honors (anyOf), so a model call using an alias is not rejected at the pre-dispatch schema boundary. - document-read.input.v1.json: reject empty and absolute paths at the schema (minLength + `^[^/]` pattern), matching the scoped-path contract. - ironclaw_memory service.rs: restore the pre-lift null-target handling — an explicit JSON `null` write target is treated as omitted (daily_log) rather than rejected, the #4547 behavior the lift had regressed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): green the Reborn e2e harness + coverage for the native package Moving the memory_* capabilities into the `ironclaw.memory.native` package left two root-crate tests red, because the e2e harness and the e2e coverage allowlist still assumed memory lived in `builtin`: - reborn_trace_first_party_tool_coverage.rs: build the covered-capability set from the union of the builtin and native-memory packages, so the always-on first-party surface (which now spans two packages) is fully checked. - tests/support/reborn/harness.rs: register `native_memory_first_party_package` in the core-builtins runtime (via a shared `core_builtins_extension_registry` so the two core-builtins runtimes cannot drift), trust the native provider at the host-policy and per-run authority levels, and scope memory grants to filesystem effects so they fit the native provider's tight authority ceiling. Fixes `reborn_builtin_first_party_capability_e2e_coverage_is_complete` and `reborn_trace_memory_first_party_tools_parity` on `Reborn root tests` and `Tests (all-features)`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(memory): config-driven mem0 document-store provider (#3537) Make the memory document-store provider swappable to mem0 entirely through config, with no hardcoded native assumption in the kernel — demonstrating the #3537 memory architecture is genuinely pluggable. Follows the established `ironclaw_embeddings::create_provider` config-driven-factory idiom. - ironclaw_host_runtime/memory_provider.rs: remove the hardwired native-or-none logic; `MemoryServiceResolver` is now a provider-agnostic registry (`BTreeMap<extension_id, Arc<dyn MemoryService>>` + a `with_third_party_document_store_provider` builder). `resolve_document_store` matches the binding (Native -> build native / ThirdParty(id) -> registered instance, None if unregistered / Disabled -> None). It names no concrete third-party provider, so host_runtime keeps zero provider deps. - crates/ironclaw_memory_mem0: new provider crate implementing MemoryService over the mem0 REST API. Real reqwest transport behind a `Mem0Transport` trait (SSRF-checked base URL, `Authorization: Token` header) with a panic-free mock for tests. Depends only on ironclaw_memory + ironclaw_host_api. - ironclaw_reborn_composition: `create_document_store_provider(binding, deps)` factory (embeddings idiom: match Native/ThirdParty/Disabled, build mem0 over its real transport with check_base_url, fail-closed None on missing creds) plus `build_memory_service_resolver` that registers third-party providers; wired into all three resolver-construction sites in factory.rs. - Config: `[memory] mem0_base_url` + env MEMORY_MEM0_{API_KEY,BASE_URL,APP_ID} (API key as SecretString), mirroring EmbeddingsConfig. - Tests: end-to-end swap proof (config -> policy -> factory -> registry -> resolve_document_store returns mem0, not native; write+search route through the mem0 mock transport), mem0 unit tests, and a caller-level tool-dispatch test proving the unchanged ironclaw.memory.native.* tools transparently route to mem0 under a binding. Architecture boundary allowlist updated for the new crate (composition may depend on it; host_runtime may not). Scope: document-store swap only. The host-managed retrieve/record lifecycle and SQL storage-port backing remain the deferred #5264 follow-ups. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(memory): host drops provider-supplied cross-scope context snippets Add a full-pipeline (`load_memory_snippets`) negative test proving the host drops memory-context snippets whose resolved tenant/user scope does not match the request scope, even when the provider returns them — keeping only the in-scope snippet. The scope guard was unit-tested at the `admit_*` level; this exercises it end-to-end against a malicious or buggy provider, which is now a live possibility with config-bound third-party providers like mem0 (#5264). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(memory): make the mem0 provider fully local (self-hosted OSS) Re-target the mem0 document-store provider from mem0's hosted cloud API to a self-hosted mem0 open-source instance on localhost — no dependency on api.mem0.ai and no cloud API key. Proven end-to-end against a real local stack (mem0ai + Qdrant + an Ollama embedder): store -> search-recall -> verbatim read round-trips, with the data physically in the local vector store. - transport.rs: the API key is now optional (`Option<&str>` — the auth header is sent only when a key is present); add `.timeout(30s)` + `.redirect(none)` (review hardening, finding #2). - service.rs: local OSS paths (`/memories`, `/search`, `GET /memories?user_id=`) instead of the hosted `/v1/memories/...`; `add` sends `infer:false` so mem0 stores content verbatim (document-store semantics need only the embedder, not the extraction LLM). `profile_set` is now field-preserving (read-merge-write, latest selected by `created_at`) instead of last-writer-wins (review finding #1 — no more silent profile-field loss); merge + infer-false unit tests added. - lib.rs: extension id `mem0.cloud.memory` -> `mem0.local.memory`; docs rewritten to the local OSS surface. - composition factory: build the provider with an optional key (no longer fails closed on a missing key); reborn_cli defaults `MEMORY_MEM0_BASE_URL` to `http://localhost:8888`; config doc updated. - swap-test fixtures updated to the local API; new `tests/live_local_mem0.rs` (`#[ignore]`'d) drives the real transport against a running local mem0. The document-store swap is proven at the provider level. The full LLM-agent loop using memory remains blocked on this branch by the pre-existing #5206 worker-pool stall (fixed on main) and is a tracked follow-up (#5264). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): keep mem0 off by default (no default base URL) Remove the `http://localhost:8888` default for the mem0 connection base URL so mem0 is fully opt-in: it activates only when an operator both binds the document-store profile to it AND supplies a base URL (the `[memory]` config or `MEMORY_MEM0_BASE_URL`). A bound-but-unconfigured mem0 fails closed in the factory, and the binding policy already defaults to native — so the shipped default memory layer is unchanged (native filesystem); mem0 never engages unless explicitly configured. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): apply consolidated review findings (docs, secrets, fail-loud, tests) From a 5-agent end-to-end review of the PR. All low-risk: - mem0 provider: reject embedded-credential base URLs (redacted in the error) and run `memory.mem0_base_url` through the config inline-secret guard; correct the stale "defaults to localhost:8888" doc-comments (there is no default — a bound-but-unset mem0 fails closed); add the missing `created_at` newest-profile test; fail loud (`CorruptProfile`) instead of silently dropping fields when an existing profile blob is unparseable; add the `// silent-ok:` annotation and drop cloud (`api.mem0.ai`, `/v1/`) remnants from test/doc strings. - reborn_cli: `optional_nonempty_env` fails loud on a non-UTF-8 value (NotPresent -> None, NotUnicode -> Err) for the three MEMORY_MEM0_* reads. - reborn_config: deployment-profile validation uses `RebornProfile::from_str` instead of matching string literals (types.md). - architecture: add a boundary-test guard asserting host_runtime stays memory-provider-neutral (only composition may name `ironclaw_memory_mem0`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(memory): document that prompt-write-safety is native-only (#5264) A third-party document_store binding (e.g. mem0) does not get write-time prompt-write-safety enforcement or per-write audit, since that engine lives inside the native provider. Spell out the security limitation in resolve_document_store + why it is acceptable for the off-by-default surface (third parties cannot reach the trusted prompt surface; all retrieved content is host-wrapped untrusted), and that hoisting it host-side is deferred to #5264. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(memory): feature-gate the mem0 provider behind `memory-mem0` (off by default) mem0 is now opt-in at build time, mirroring ironclaw_llm/root-llm-provider: ironclaw_memory_mem0 is an optional dependency enabled by a new `memory-mem0` feature on composition, so a default build carries no mem0 code or its reqwest/rustls transport. The factory's mem0 construction (plus its test seam, tests, and the swap integration test) are #[cfg(feature = "memory-mem0")]; a mem0 binding fails closed when the feature is not compiled in. The architecture boundary test asserts the gating (mirroring root-llm-provider), and CI runs the composition suite with the feature so the mem0 tests still execute (feature-off stays covered by the --no-default-features composition run in test.yml). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address CodeRabbit re-review findings (mem0 correctness, schemas, secrets) From CodeRabbit's full re-review of the rebased PR. All low-risk: - mem0 provider: read fragments now sort by created_at (mem0 list order is not chronological) so append-style docs read back in order; a replace write (append=false) is rejected as an Unsupported operation instead of silently becoming an add that misreports append:true; check_base_url fails closed on hosted mem0 cloud hosts (mem0.ai / *.mem0.ai), enforcing self-hosted-OSS-only; the InvalidUrl error no longer echoes the configured URL (drops host/query), keeping only the cause. - reborn_config: mem0_base_url is validated non-empty + trimmed (check_non_empty_trimmed). - native memory schemas: tree.input rejects absolute / .. / backslash paths (fail closed, empty root still allowed); document-read.output requires word_count; search.input requires a non-empty query and forbids conflicting aliases (oneOf). - docs: memory-profiles.md non-goals updated (mem0 provider now exists, off by default, feature-gated); host_port.rs docstring no longer over-claims native backing (native memory is filesystem-backed, declares no host ports). - memory_binding: regression test locking that a case variant of the native id fails closed (ExtensionId grammar is lowercase-only) rather than misclassifying. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address CodeRabbit re-review round 2 (target containment, fail-loud, docs) From CodeRabbit's fresh full re-review. Most notable: the native memory JSON input schemas are model-facing (advertised in parameters_schema) and are NOT host-validated against actual tool arguments before dispatch, so a traversal target could reach a provider verbatim. Added a provider-neutral reject_out_of_scope_target guard in MemoryServiceWriteRequest::from_tool_input (mirrors the schema pattern) so every bound provider -- including mem0, which stores the target verbatim -- gets containment, not just native. Also: - document-write.input schema rejects absolute / .. / backslash targets (fail closed, matching the sibling schemas). - mem0 response_items fails loud (UnrecognizedResponse) on an unrecognized 2xx body instead of silently returning empty (which let a malformed list response overwrite existing profile fields). - docs: manifest description scoped (search/tree implement no portable profile); extension_contracts + transport docstrings corrected (native is filesystem- backed / declares no host ports; non-JSON bodies degrade to Null, not an error). The mem0 replace-write rejection is provider-specific (mem0 OSS is append-only); native still supports replace, so the shared write prompt is left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(memory): forward the memory-mem0 feature from the reborn CLI Completes the mem0 feature-gate: ironclaw_reborn_cli now exposes a memory-mem0 feature that enables ironclaw_reborn_composition/memory-mem0, so an ironclaw-reborn binary built with --features memory-mem0 can bind memory.document_store.v1 to a self-hosted mem0 server. Without it the feature was only reachable on the composition crate, never the actual binary. Mirrors the existing root-llm-provider / webui-v2-beta forwarding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(deps): ignore RUSTSEC-2026-0187 (lopdf PDF stack-overflow, bounded DoS) New advisory on lopdf (via pdf-extract in ironclaw_extractors, PDF attachment extraction) with no patched release in our semver range yet. Bounded DoS only -- a crash of the extraction task on a hostile user-supplied PDF, not RCE and not the whole process. Matches the existing deny.toml ignore pattern; remove once lopdf/pdf-extract ship a fixed release. Unrelated to the memory work; surfaced because the main merge re-ran cargo-deny. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(memory): workspace-bound local-dev mem0 isolation (fold workspace + scope into user_id) Local-dev native memory is isolated per workspace (its filesystem store lives under local_dev_root); mem0 (a shared server) was not, since the local-dev runtime uses a fixed scope. mem0 OSS enforces search/get_all filtering by user_id (and agent_id) but NOT by app_id -- a top-level app_id is accepted yet silently ignored when filtering (verified empirically: cross-app_id queries leak). So we encode the entire partition into the one key guaranteed enforced: - The provider folds the workspace partition (config.app_id, set per-workspace by the local-dev composition from local_dev_root) into the user_id namespace at all 7 namespace sites (search/write/read/tree/profile-read/profile-set/retrieve_context). app_id is still stamped as forward-compat metadata but is NOT relied on for isolation. - The local-dev composition derives a per-workspace app_id from the canonical local-dev root; production is untouched (no app_id -> pure scope namespace, so memory persists across restarts for the same scope). Result: local-dev mem0 partitions like native (workspace x tenant/user/agent/project). Verified by a live cross-isolation test (cross-workspace AND cross-scope both return zero on search and list) + a regression guard locking the user_id prefix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): make agent-facing memory surface backend-agnostic (ironclaw.memory.*) The bundled memory extension exposed its model-facing tools as `ironclaw.memory.native.*`, leaking the default provider's name into the one surface that must be backend-blind — the agent's tool list. With the mem0 swap the model still saw `ironclaw__memory__native__*` while running on mem0, contradicting #3537's agnostic goal. Rename the agent-facing extension id + capabilities to `ironclaw.memory.*` (model now sees `ironclaw__memory__{read,search,write,tree}`) and neutralize the manifest name/description. "native" is kept only where it's true — the internal default provider: `native_memory_provider` service, `ironclaw_memory_native` crate, `NativeMemoryService`, the bundled asset/prompt dirs. The last dot-segment (read/search/write/tree) is preserved, so the benchmark retrieval matcher (keys on `rsplit('.').next()`) is unaffected. Tests: host_runtime memory/manifest/surface, capability-profile conformance, and reborn_composition projection all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address userland extension review feedback * Fix native memory CI harness wiring * feat(memory): host-managed memory lifecycle — two-lane retrieval + after-turn record (mem0 flow) (#5327) * feat(memory): host-managed memory lifecycle — two-lane retrieval + after-turn record (mem0 flow) Implements the mem0 host-managed memory flow on top of #5205, with the surface area confined to the native memory provider and run-level orchestration. Retrieval (once per run): - The loop fetches long-term (user-general) and short-term (per-thread, `threads/<thread_id>/`) memory once at the first prompt build of a run and injects both into the prompt's "memory" section, replacing the dead `memory_snippets: Vec::new()` in loop_support. A per-run OnceCell caches the fetch; subsequent model steps reuse it. - Native `retrieve_context` scopes by `invocation.scope.thread_id`: Some(T) → only that thread's `threads/<T>/` subtree (short-term); None → the user's general memory, excluding `threads/*` (long-term). The lanes are disjoint, so the host concatenates them (short-term first) under the existing 4 KiB admission budget. - Graceful degradation throughout: a memory failure degrades the lane to empty and never breaks a turn. Recording (after each turn): - New low-level `MemoryService::record_interaction(invocation, { messages, run_id, metadata })` — the mem0 `add` data shape (`user_id`/`agent_id`/ `thread_id` ride the invocation scope). A default no-op trait impl lets each provider opt in: the host passes the DATA and the provider decides what to do with it (store verbatim, run LLM extraction, or nothing). Implements the reserved `memory.interaction.record.v1` vocabulary. - The native provider stores the full turn history under `threads/<thread_id>/`. - A host `AfterTurnMemoryRecorder` fires at the run-end seam (`turn_run_executor::apply_exit`, gated on `Completed`), reads the exchange from the thread transcript with the owner-rewritten scope, and hands it down. Post-terminal and best-effort: every error is `debug!`-logged and never fails the already-completed run. Reads and writes resolve on the local-dev runtime path; the production graph wires `None` (deferred, issue #5013 — the same optionality as `user_profile_source`). Tested at unit + caller level across ironclaw_memory{,_native}, host_runtime, loop_support, turns, and reborn (two-lane fetch, prompt rendering, once-per-run cache, native record→retrieve, and a full-turn record through the executor). A full-composition e2e (record in one run → surface in a later run's model request) is called out as a follow-up. Design: docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address review — full transcript (H1), run-vs-session, idempotency, build break Addresses the adversarial audit + the mem0 data-parity audit + CodeRabbit on #5327. - H1 + parity (transcript): `build_exchange` → `build_transcript` now captures the FULL ordered run transcript — every user/assistant/tool message of the turn, in sequence order, every finalized assistant including the FINAL answer (fixes recording the first/intermediate assistant on multi-step runs), each tagged with its actor `name`. This is the data mem0's `add` receives. - Run-vs-session (parity): rename `MemoryServiceRecordRequest.run_id` → `turn_run_id` (per-turn provenance). mem0's session id maps to `scope.thread_id` (the conversation), NOT this field — documented on the contract + recorder so a mem0 provider can't mis-map (which would write under the turn id but read under the session id → silent short-term-recall miss). - Idempotency (CodeRabbit): native `record_interaction` writes the transcript to a per-run file `threads/<thread_id>/<turn_run_id>.md` with `append: false` (overwrite), so a scheduler re-run of an already-Completed run can't duplicate the exchange or grow an unbounded `log.md`. - Parity: add `MemoryInteractionMessage.name` (mem0 message name → per-memory `actor_id`); populate `metadata` with `{turn_run_id, correlation_id}` provenance. - Build break (CodeRabbit, critical): add the missing `after_turn_memory_writer` field to the root crate's `tests/support/reborn/harness.rs` (was `E0063`). The gate now compiles the root crate's reborn tests. - Audit M1: the per-run memory `OnceCell` no longer freezes to empty when the first prompt build has no user message — it seeds only once a real request exists. - Audit L3: `// arch-exempt: optional_arc` annotations on the new optional fields; corrected the misleading "mirrors user_profile_source" comments. - Docs: long-term lane is full-tuple `(tenant,user,agent,project)` scoped; the `threads/` reservation is advisory (L1); fetch-once-per-run has no mid-run invalidation in v1 (Q6). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address re-review — lane starvation, threads/ write-reject, no transcript trim, render ordering, fetch-once-per-run CodeRabbit: - retrieve_context: over-fetch BEFORE the short-term thread-lane filter, then truncate, so general hits in the global top-N can no longer starve the thread-scoped lane (TDD). - write: reject the reserved `threads/` namespace (fail loud). record_interaction still writes there via a new private write_reserved_document bypass (TDD). Un-defers audit L1. - build_transcript: stop trimming message content — filter blank-only rows but pass content through verbatim ("LLM data is never deleted") (TDD). - InstructionBundleBuilder::build: preserve the host's short-term-first memory order; drop the by-ref re-sort that scrambled lane priority at the render boundary (TDD; updates the now-obsolete model_content-tiebreak test). - loop_driver_host: caller-level test through build_default_planned_runtime that the after_turn_memory_writer is plumbed into the executor (not just the builder shortcut). - threadless record_interaction test: supply a real turn_run_id to isolate the no-thread branch; soften the after-turn writer contract comments (runtime.rs + after_turn_memory.rs) to match the full-transcript behavior. Gemini: - load_memory_snippets_once: get_or_init + cache empty on failure = true fetch-once-per-run, no retry-storm on a slow/down memory service (M1 early return preserved). - memory_context: share one correlation_id across both retrieval lanes. Also: strengthen the aggregate-budget test with a non-empty assertion (so the all-short-term check isn't vacuous), and align the design doc with shipped behavior (no mid-run invalidation; full transcript; threads/ enforced). cargo fmt + clippy clean (zero warnings); per-crate tests green: memory_native, host_runtime, turns, loop_support, reborn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address re-review round 3 — non-blank query, reserved-write guard, bounded recorder, silent-ok, test race CodeRabbit (round 3): - latest_user_message_text: return the latest NON-BLANK user message so a blank trailing user turn doesn't drop proactive memory for the run (+ unit test). - write_reserved_document: enforce the `threads/` bypass — reject any non-threads resolved path so the public `write` guard can't be circumvented via this helper. - after_turn_memory: annotate the two intentional post-terminal fallbacks with `// silent-ok:` per the fail-loud convention. - turn_run_executor: bound the inline after-turn recorder await with a 30s timeout so a slow/hung provider can't occupy the scheduler worker. - loop_driver_host: move the sibling test's scheduler shutdown after the memory read/asserts so it can't race the recorder. - reborn_composition: soften the after-turn `[user, assistant]` contract comment to match full-transcript behavior (same fix as runtime.rs). fmt + clippy clean; tests green (loop_support 336, memory_native 20, reborn 258 + loop_driver_host 109). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address re-review round 4 — fetch memory lanes concurrently retrieve_context awaited the short-term and long-term lanes sequentially; they are independent (share only the Copy correlation id), so fetch them with tokio::join! to cut added run-start prompt-path latency, keeping short-term-first concatenation. (CodeRabbit, trivial.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(memory): self-contained read_long_term/read_thread on MemoryService Fold the prompt-context lane orchestration into the memory service (review feedback): the host no longer knows about "lanes." Two new provider-agnostic default trait methods own both the lane scoping AND the per-snippet safety: - read_long_term: clears the thread sub-scope (general memory), then sanitizes each candidate (scope-check + control-strip + size-cap + untrusted envelope). - read_thread: keeps the thread sub-scope (this conversation), same safety. Providers implement only the raw retrieve_context; they inherit the safe lane reads, so no provider can return unsafe memory context. ironclaw_memory gains a leaf dependency on ironclaw_prompt_envelope (no cycle); the sanitize helpers + scope check move there with their unit tests. The host ProductionMemoryPromptContextService collapses to two scoped reads + a trivial map to LoopContextSnippet (net -322 lines). The map keeps only the two loop-layer steps that can't move down without a dependency cycle: the model-visible reference and the loop's prompt-content denylist drop-filter (LoopSafeSummary), a prompt-layer policy applied to all model context. Deleted: the MemoryLane enum, retrieve_lane, admit_memory_context_snippet, ExpectedSnippetScope, and the host-side sanitizer. Behavior change: prompt order is now long-term then short-term (this conversation nearest the current message, per the mem0 on_run_start shape), which also flips which lane wins under the shared 4 KiB budget (was short-term-first). read_profile is unchanged: profile_read already returns the raw doc and the host parses it into the loop-shaped UserProfileContext — the correct provider-neutral split, not the lane confusion this refactor targets. fmt + clippy clean; tests green across ironclaw_memory, ironclaw_memory_native, ironclaw_host_runtime; consumers (mem0 / reborn / reborn_composition) compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(memory): address host lifecycle review feedback --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(memory): model memory as a v3 [memory] adapter with native + mem0 backends Migrate the userland memory extension to Extension Manifest v3 and retire the v2 capability-profile machinery, replacing it with a first-class [memory] adapter surface (issue #3537 / #5264). - v3 manifests for both backends: `ironclaw.memory` (native, filesystem-backed) and `mem0.local.memory` (mem0 OSS), each declaring `[memory] operations = ["document_store","context_retrieval","interaction_log"]`. - New `MemoryDescriptor` / `MemoryOperationKind` in `ironclaw_host_api::memory`. The `MemoryService` trait is the implementation-agnostic adapter the agent loop and the `ironclaw.memory.{read,write,search,tree}` tools talk to; the tool ids the model sees never change when the backend does. - Native + mem0 are interchangeable backends behind the adapter, selected by a compile-time `[memory]` binding (`memory-mem0` feature; fail-closed native default; no runtime swap). `MemoryBindingPolicy` collapses to a single `MemoryProviderBinding`; config `MemorySection`: `profile_bindings` -> `provider`. - Memory rides the always-on first-party lane (not the installable catalog), so it never appears as an installed extension. - Remove the now-dead capability-profile vocabulary: `CapabilityProfileId` / `CapabilityProfileContract` / `CapabilityProfileOperationContract`, the `ironclaw_capabilities` conformance harness, and the capability `implements` field (zero production readers; the adapter trait is the contract now). Keep `CapabilityProfileSchemaRef` (load-bearing for every capability's schema refs). Fix a group_memory regression the change surfaced: v3's `with_dispatch_effect` adds `DispatchCapability` to every memory tool (matching every other first-party tool -- echo/http/shell/...), so the test-harness trust ceiling must grant it too or every memory dispatch is `PolicyDenied`. Production ceilings already did. Verified: cargo fmt; workspace clippy --all-targets --all-features -D warnings (exit 0); architecture boundaries + manifest-reparse gate; group_memory (13); mem0<->native swap (5); host_runtime memory (382); reborn_composition (1402); default + all-features compile lanes. Pre-existing (base-confirmed, unrelated): factory local_dev_memory (2, FilesystemDenied) + sandbox_process (3, no Docker). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ9i53L6mBFnnsvtPSrSzc * fix(ci): expect memory-mem0 in composition feature-flags self-test The memory PR added the off-by-default `memory-mem0` feature to `ironclaw_reborn_composition` and updated package-feature-flags.sh to emit `--features test-support,memory-mem0` for it, but the self-test's pinned expectation still read `--features test-support`. Update the assertion to match the script it guards. The change IS the test update, so no separate regression test applies. [skip-regression-check] Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ9i53L6mBFnnsvtPSrSzc * test(host-runtime): memory tool descriptors carry DispatchCapability post-merge main's manifest reparse gate + `with_dispatch_effect` apply DispatchCapability to every v3 first-party tool descriptor (consistent with builtin http et al.), so native memory's descriptors are now [DispatchCapability, ReadFilesystem, ...]. Update the effect assertions to match, and drop the memory ids from the builtin origin-gate-matrix spot-checks since memory moved to the standalone ironclaw.memory package. [skip-regression-check] Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ9i53L6mBFnnsvtPSrSzc * fix(memory): declare origin-gate matrices + registry-lane allowlist for the ironclaw.memory package Two masked layers made every ironclaw.memory.* dispatch fail in any composition with an extension host installed: 1. The memory tools moved out of the builtin package (which stamps OriginGateMatrix::builtin_loop_run_seed on every Rust-built descriptor) into the hand-authored memory_native v3 manifest, which declared no origin_gate_matrix. The S4 authorize fold fail-closes a missing matrix to Forbidden for every origin-stamped invocation, so model dispatch died with Authorization (PolicyDenied). Declare the behavior-preserving matrices in the manifest: read/search/tree stay Ungated for LoopRun via the reviewed allowlist (renamed from the retired builtin.memory_* ids, count unchanged), write stays gated_unless_granted, product/automation stay forbidden. 2. Once authorized, dispatch still failed UnknownCapability: the registry-lane provider allowlist (applied when the extension-host snapshot resolver cuts over) listed only the builtin provider, but the always-on ironclaw.memory package resolves through the same registry lane and is never published in the extension-host snapshot. Add the native memory provider to the allowlist. Regression coverage: factory::tests::local_dev_memory_* (caller-tier, red before this fix) now pass; native_memory_package_declares_behavior_neutral_origin_gate_matrix pins the descriptor-tier matrix contract that was dropped when memory left the builtin package; the origin-gate ratchet's TOML scan now also walks crates/ironclaw_host_runtime/assets so a host-bundled manifest can never again ship without a matrix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1q1QYdRY3XRg6RnYLZdNw * fix(ci): give QA smoke binaries the documented RUST_MIN_STACK headroom The reborn_qa_smoke_scenarios_e2e binary builds a full Reborn runtime and drives whole turns on the libtest current-thread stack; with this branch's memory pipeline additions its combined debug async frames overflow the 8 MiB default test-thread stack (measured need ~10 MiB; CI aborts with "fatal runtime error: stack overflow" in root tests (1), locally reproducible on qa_installing_bundled_extensions_exposes_complete_model_surface_e2e). Set RUST_MIN_STACK=64 MiB on the root-tests job — the same value and pathology reborn_qa_recorded_behavior.rs already documents for its recorder ("builds two runtimes plus a live turn, whose combined debug async frame overflows the default test-thread stack") — and document the requirement on the smoke binary itself. The full 26-test binary passes under this value. [skip-regression-check] CI environment headroom for a debug-profile stack exhaustion; the binary's existing tests are the regression surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1q1QYdRY3XRg6RnYLZdNw * test(reborn): finish builtin.memory_* -> ironclaw.memory.* rename in composition tests + re-bless surface hash Two stragglers from the memory-package rename that only fail in CI lanes wider than --lib: webui_v2_e2e asserted builtin.memory_write is visible in the local-dev capability surface (composition-core bucket), and the golden payload snapshots pinned the pre-origin-gate-matrix surface sha256 (integration coverage lane 3). Rename the ids and re-bless; the snapshot diff is exactly the surface-hash token — the capability list, names, and descriptions are unchanged. Also rename the opaque id in the local_dev result-staging test for consistency. [skip-regression-check] test-only rename + snapshot re-bless; the renamed assertions are themselves the regression coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1q1QYdRY3XRg6RnYLZdNw * test(reborn): align channel-connection projection with the #6618 retain-generated-code contract Main's #6618 refined the extension_search sanitizer to RETAIN a generated-code channel's setup guidance (blanking only the static failure copy) and pinned that in a unit test — but left the integration test asserting the old strip-everything contract. The contradiction was invisible on main because main's tip cannot compile the integration-test closure at all: #6618 renamed the RebornRuntime field to `_channel_host_assembly` but missed the test-support-gated accessor (`active_channel_preference_codec_ids_for_test`), so every integration-tier suite fails at compile and all five coverage lanes are red on main. This branch already carries the accessor fix from the catch-up merge; this commit carries the test-contract fix: telegram's web_generated_code guidance must remain model-visible with "IronClaw pairing panel" instructions and a blanked error_message, mirroring model_visible_extension_search_projects_generated_code_without_ui_failure_copy. [skip-regression-check] test-only contract alignment; the rewritten assertions are the regression coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1q1QYdRY3XRg6RnYLZdNw * review(memory): apply CodeRabbit/Gemini findings across the memory surface Verified each open review thread against current code; fixes for the still-valid ones: - Harness trust parity (security): the integration harness granted the ironclaw.memory provider the FULL builtin effect set (Network/SpawnProcess/ExecuteCode/...); production grants only dispatch + filesystem. Narrow core_builtin + qa_smoke profiles to the production ceiling so harness runs can't mask authority-ceiling denials production would enforce. - mem0 retrieve_context now filters the kind=profile record out of context snippets (profile JSON must not enter the prompt as a memory snippet; profile state has its own read path) + regression case. - Caller-level proof of the retrieve-before lane: drive the REAL LoopContextPort::load_loop_context twice with a recording MemoryPromptContextService — asserts the query is the latest user message, snippets surface on the bundle, and the fetch happens once per run (cache reuse). - [memory] manifest validation regression tests: non-first-party runtime, empty operations, and missing document_store all fail closed (+ a parsing baseline for the provider-only shape). - surface.rs: mirrored fail-closed test — a native-memory descriptor without an input schema ref is rejected like a builtin one. - Origin-gate ratchet now asserts BOTH host-bundled memory manifests are actually scanned (a moved manifest can no longer silently drop out). - document-read input schema mirrors write/tree's stricter path not-pattern (blank/absolute/traversal/backslash); tree output schema types its items as path strings. Verified-invalid threads (no change): the "duplicate [[tools]] headers" critical is a diff-context misread (one header per tool; the package parse is pinned by tests); the memory_provider_factory CapabilityProfileId silent-drop refers to code removed with the capability-profile vocabulary retirement. Deferred: mem0 unbounded list pagination (off-by-default provider; needs a mem0 API paging design). [skip-regression-check] review-driven test hardening; each behavioral fix above carries its regression case in the same commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1q1QYdRY3XRg6RnYLZdNw --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Robert Yan <46699230+think-in-universe@users.noreply.github.com>
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
1 participant
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.
Reborn: model memory as a userland extension — implements #3537
Implements the bulk of #3537 — Extension Manifest v2 architecture, source-aware trust, host-defined capability profiles + conformance, the memory profile-binding policy, and the always-on native document-store provider. It also includes a config-driven third-party document-store provider backed by a self-hosted, fully-local mem0 (mem0 OSS on localhost — no api.mem0.ai, optional key), demonstrating the provider is swappable entirely through config with no hardcoded native assumption (the
host_runtimeresolver names no concrete provider; construction follows theironclaw_embeddings::create_provideridiom in composition). Proven end-to-end at the provider level against a real local mem0 stack (store -> search-recall -> verbatim read). It does not fully close #3537: native SQL storage-port backing (reborn_memory_*tables), the host-managedmemory.context.retrieve/memory.interaction.recordflow, the default flip +/memorymigration, andmemory.semantic_search.v1remain follow-ups tracked in #5264.Memory is now a host-bundled userland extension that implements host-defined capability profiles, bound per profile through a fail-closed
profile_id → extension_idpolicy — consolidated to a single, always-on native surface.End state: one always-on native memory extension
ironclaw.memory.nativeis a bundled v2 Extension Manifest (schema_version = "reborn.extension_manifest.v2") parsed fromcrates/ironclaw_host_runtime/assets/memory_native/manifest.tomland registered on the always-on first-party lane (the same lane as the builtin toolset) — not the catalog/lifecycle lane — so its tools are unconditionally available with no install/enable step. It declares fourmodel-visible memory tools (read/write/search/tree);read/writeimplementsmemory.document_store.v1(their schema refs match the profile's operation refs), andsearch/treeare native conveniences that implement no profile. The model-facing tool names areironclaw__memory__native__{read,write,search,tree}.This replaces the prior two declarations of the one filesystem-backed provider — the live
builtin.memory_*tools and a separate, dormanthost_internalnative manifest — with one.builtin.memory_*is removed; provider-swapping is governed by the document-store profile binding (config-driven), not by install/enable.The live provider is filesystem-backed and declares no host ports; input schemas are served inline (
include_str!of the bundled asset files, the single source of truth) on the always-on lane rather than materialized. Behavior, I/O, the scoped/memorymount, and on-by-default availability are unchanged frombuiltin.memory_*; only the capability identity + derived tool names change.Kept #3537 machinery
ironclaw_host_runtime::memory_profiles): the three host-defined contracts (memory.context_retrieval.v1,memory.interaction_log.v1,memory.document_store.v1) and the semantic conformance harness. The nativeread/writecapabilities are proven to satisfymemory.document_store.v1.default_host_port_catalog()):host.storage.sql_transaction.first_party+host.events.auditstay catalogued as deferred-future vocabulary.MemoryBindingPolicy, fail-closed; the[memory]config section;MemoryServiceResolversingle construction point): default-native, production rejectsmemory.disabledand unverified third-party bindings absent an(extension_id, profile_id, deployment_profile)admin override.MemoryServicefacade.Deferred (not stubbed)
context_retrieval/interaction_logare defined and conformance-testable but have no live implementer; wiring the host turn pipeline to invokememory.context.retrievebefore model calls and record sanitized interactions viamemory.interaction.recordafterward is deferred. The live native surface ships only the model-facing document-store tools.reborn_memory_*dual-backend tables behindhost.storage.sql_transaction.first_party, behind non-defaultmemory-native-*features (the reborn-composition crates cannot depend on the rootironclawcrate where the Postgres/libSQL backends live). Seedocs/adr/0002-native-memory-uses-host-storage-ports.md.memory.semantic_search.v1— unchanged from the issue's deferral (/memorydata/API compatibility decision; host-mediated embedding port).Gates
cargo fmt --all -- --check— cleancargo clippy -p ironclaw_host_runtime -p ironclaw_reborn_composition --tests --features "root-llm-provider webui-v2-beta libsql"— zero warningscargo test -p ironclaw_host_runtime— green (the 3sandbox_processfailures are pre-existing, Docker-only)cargo test -p ironclaw_reborn_composition --features "root-llm-provider webui-v2-beta libsql"— green (the 1skill_learned_bubblefailure is pre-existing under this feature combo onmain)cargo test -p ironclaw_architecture— green (no new cross-crate dependency edges)python3 scripts/check_no_panics.py --base main --head HEAD— OK🤖 Generated with Claude Code