feat(memory): model memory as a userland extension (#3537) - #5163
Merged
Merged
Conversation
|
This PR was not deployed automatically as @BenKurrek does not have access to the Railway project. In order to get automatic PR deploys, please add @BenKurrek to your workspace on Railway. |
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesMemory contract/native separation and
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 restructures the memory architecture by splitting ironclaw_memory into a provider-neutral contract crate and a native provider implementation crate (ironclaw_memory_native), adapting the host runtime to route memory operations through the new MemoryService facade. It also updates manifest validation to require prompt_doc_ref for model-visible and API-visible capabilities. The review feedback highlights opportunities to clean up unused helper functions in the native service, optimize string length checks in locale validation by using byte length instead of character count, and mitigate potential key collision risks in snippet reference generation with structured keys or explicit collision logging.
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
force-pushed
the
reborn/memory-placement-m2-lift
branch
from
June 23, 2026 18:56
ab24f9e to
d4b5337
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
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/src/events.rs (1)
158-175: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a named search-mode type for event construction.
Two adjacent
bools can be swapped while compiling cleanly, corruptingfull_text/vectortelemetry. Pass a small named struct or enum instead.As per coding guidelines: “Use enums for units, shapes, and modes … instead of booleans plus magic strings.”
Proposed shape
+#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct MemorySearchMode { + pub full_text: bool, + pub vector: bool, +} + pub fn search_performed( scope: &MemoryDocumentScope, source: MemorySignificantEventSource, - full_text: bool, - vector: bool, + mode: MemorySearchMode, result_count: u64, ) -> Self { @@ - full_text: Some(full_text), - vector: Some(vector), + full_text: Some(mode.full_text), + vector: Some(mode.vector),🤖 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/events.rs` around lines 158 - 175, The search_performed function has two adjacent boolean parameters (full_text and vector) that could be accidentally swapped during refactoring, corrupting telemetry data. Create a named struct or enum type to represent the search mode configuration that encapsulates both the full_text and vector search parameters. Replace the two separate bool parameters in the search_performed function signature with a single parameter of this new named type, and update the function body to access the full_text and vector values from the named type instead of directly from parameters.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/first_party_tools/memory.rs`:
- Around line 444-463: The write_response_to_value function uses string matching
on the status field of MemoryServiceWriteResponse instead of a typed enum, which
allows provider contract drift to silently change the tool output. Create a
typed enum in the ironclaw_memory crate to represent the write response status
(with variants for "cleared", "patched", and other valid states), change the
status field in MemoryServiceWriteResponse from String to this enum type, and
then update the match statement in write_response_to_value to match exhaustively
on the enum variants instead of string literals. This ensures all possible
status values are explicitly handled and any unexpected status values will be
caught at compile time.
In `@crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs`:
- Around line 33-35: Reorder the authorization check before input parsing in the
profile_set function. Move the ensure_memory_mount call to execute before the
MemoryServiceProfileSetRequest::from_tool_input parsing operation, so that write
permission to the /memory mount is validated first. This ensures the code fails
closed with FilesystemDenied error for unauthorized callers rather than
revealing input validation details through error feedback.
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 93-112: The memory context validation logic is using warn-level
logging which can corrupt the REPL/TUI terminal display, and is silently
dropping LoopSafeSummary validation failures via the .ok()? pattern. Replace all
four tracing::warn! calls with tracing::debug! to designate these as internal
diagnostics. For the two LoopSafeSummary::new() calls that currently use .ok()?,
replace the .ok()? pattern with explicit match statements that log the
validation error before returning None, ensuring validation failures are visible
in debug output rather than silently swallowed.
In `@crates/ironclaw_memory_native/src/metadata.rs`:
- Around line 32-50: The metadata parsing in this resolver silently drops errors
through merge() and DocumentMetadata::from_value(), which can result in lost
schema or hygiene controls being persisted. Replace the unwrap_or_else defaults
in the find_nearest_config call and the final DocumentMetadata::from_value()
chain to explicitly return a FilesystemError if parsing fails, unless the silent
failure is justified with an explicit // silent-ok: comment. Use the ? operator
to propagate errors loudly from the DocumentMetadata::from_value() call instead
of allowing it to warn and default on invalid metadata.
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 142-145: The map_err call on write_document_with_backend_options
is discarding the actual error cause with the |_| pattern, making debugging
difficult. Instead of using map_err(|_| ...), capture the error by binding it in
the closure, preserve the actual error cause for internal logging or context
(you can store it in a local variable or pass it to error context methods),
while still returning the sanitized MemoryServiceError::operation() to maintain
the public API contract. Apply this same pattern to all other similar map_err
calls that discard error information throughout the file at the locations
mentioned in the comment.
- Around line 105-117: The search method does not filter backend results by
scope before returning them, which could leak another tenant/user/project's data
if the backend has a bug. Add a scope validation filter to the search results
mapping to ensure result.path.scope() matches context.scope(), following the
same pattern already used in the retrieve_context method at Line 333. Apply the
same scope filtering fix to the tree method as well (mentioned in "Also applies
to" section) to prevent scope leakage in all paths that return backend results.
- Around line 111-115: The MemoryServiceSearchResult DTO is exposing protected
internal filenames such as BOOTSTRAP.md and HEARTBEAT.md directly in the path
field, which violates security guidelines since these DTOs are converted to JSON
for host-runtime tools. Before mapping the path using
result.path.relative_path().to_string() in the MemoryServiceSearchResult struct
initialization, filter out or map protected filenames (BOOTSTRAP.md,
HEARTBEAT.md, AGENTS.md, and .system/ paths) to aliases or suppress them from
being returned to users. Apply this same fix to all other locations where
MemoryServiceSearchResult is created, specifically at the line ranges mentioned
(146-153, 202-205, 229-232, 245-255).
- Around line 668-675: The memory_context_disabled function performs exact
string matching on context_profile_id against the MEMORY_DISABLED_ALIASES list,
which fails to match case variations like "Memory-Disabled" and could allow
memory to be incorrectly retrieved. Normalize the context_profile_id parameter
to lowercase using .to_ascii_lowercase() before passing it to the contains()
method to ensure case-insensitive matching against the alias list.
In `@crates/ironclaw_memory/src/context.rs`:
- Around line 57-64: The method `with_prompt_write_safety_enforced()` is
publicly exposed but its comment states that direct backend callers must not use
it to bypass prompt-write safety checks. This is a code/documentation mismatch
that creates a security vulnerability. Remove the public visibility of
`with_prompt_write_safety_enforced()` and either make it private to the
`MemoryBackendFilesystemAdapter` or move the `prompt_write_safety_enforced`
marker behind an adapter-only internal type that direct backend callers cannot
access. Ensure the native backend re-validates prompt-write safety rather than
trusting this marker alone.
In `@crates/ironclaw_memory/src/lib.rs`:
- Around line 6-9: The rustdoc comment in the lib.rs file incorrectly references
the implementation crate name as `ironclaw_memory` when it should be
`ironclaw_memory_native`. Update the comment that describes the native provider
implementation and storage adapters to reference the correct crate name
`ironclaw_memory_native` instead of `ironclaw_memory`, since the contract crate
cannot depend on or re-export itself.
In `@crates/ironclaw_memory/src/path.rs`:
- Around line 118-125: The public `from_validated_parts` constructor in
MemoryDocumentPath bypasses the validation that should be enforced by
`validated_memory_relative_path()`, allowing downstream crates to construct
paths with traversal sequences, control characters, absolute-style paths, or
reserved sidecar suffixes. Either make `from_validated_parts` private to prevent
external bypass of validation, or add validation logic to the function to ensure
the relative_path parameter is checked using the same validation rules as
`validated_memory_relative_path()` before construction.
In `@crates/ironclaw_memory/src/safety.rs`:
- Around line 8-9: The documentation comment for the safety module incorrectly
states that the enforcement engine lives in the ironclaw_memory crate, which is
outdated after the crate split. Update the comment at lines 8-9 to remove the
misleading reference to ironclaw_memory as the implementation crate and instead
point to the actual provider implementation crates (such as
ironclaw_memory_native) where the enforcement engine now resides to maintain
accurate cross-layer documentation.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 4-6: The module documentation comment references the incorrect
crate name for the native adapter implementation. In the comment block
describing the MemoryService trait and the default native adapter storage
behavior, replace the reference to `ironclaw_memory` implementation crate with
`ironclaw_memory_native` to accurately reflect where the native adapter and its
storage behavior are located.
- Around line 95-98: The metadata extraction logic silently filters out
non-object metadata values instead of rejecting them with an error. Replace the
filter(|metadata| metadata.is_object()) logic with explicit validation that
rejects the input and returns an error when metadata is present but is not an
object. This ensures the caller is notified of schema violations at the input
boundary rather than silently proceeding without the intended metadata context.
---
Outside diff comments:
In `@crates/ironclaw_memory/src/events.rs`:
- Around line 158-175: The search_performed function has two adjacent boolean
parameters (full_text and vector) that could be accidentally swapped during
refactoring, corrupting telemetry data. Create a named struct or enum type to
represent the search mode configuration that encapsulates both the full_text and
vector search parameters. Replace the two separate bool parameters in the
search_performed function signature with a single parameter of this new named
type, and update the function body to access the full_text and vector values
from the named type instead of directly from parameters.
🪄 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: 18f065dc-b74f-435f-ab5e-8fd6a5784015
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (74)
Cargo.tomlcrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_extensions/src/v2.rscrates/ironclaw_extensions/tests/extension_contract.rscrates/ironclaw_extensions/tests/manifest_v2_contract.rscrates/ironclaw_host_api/src/host_port.rscrates/ironclaw_host_api/tests/host_api_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/extension_contracts.rscrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/memory_profile_binding.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/host_api_contract_composition.rscrates/ironclaw_host_runtime/tests/memory_profile_binding.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_host_runtime/tests/tool_surface_contract.rscrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rsdocs/reborn/contracts/_contract-freeze-index.mddocs/reborn/contracts/extensions.mddocs/reborn/contracts/host-runtime.mddocs/reborn/memory-rd/01-memory-placement/engineering.mddocs/reborn/memory-rd/01-memory-placement/lift-implementation-plan.mddocs/reborn/memory-rd/01-memory-placement/overview.mddocs/reborn/memory-rd/01-memory-placement/research.mddocs/reborn/memory-rd/02-self-learning-write-pipeline/engineering.mddocs/reborn/memory-rd/02-self-learning-write-pipeline/lift-context-handoff.mddocs/reborn/memory-rd/02-self-learning-write-pipeline/overview.mddocs/reborn/memory-rd/02-self-learning-write-pipeline/research.mddocs/reborn/memory-rd/03-long-term-memory-retrieval/engineering.mddocs/reborn/memory-rd/03-long-term-memory-retrieval/overview.mddocs/reborn/memory-rd/03-long-term-memory-retrieval/research.mddocs/reborn/memory-rd/04-memory-benchmarks-and-evaluation/engineering.mddocs/reborn/memory-rd/04-memory-benchmarks-and-evaluation/overview.mddocs/reborn/memory-rd/04-memory-benchmarks-and-evaluation/research.mddocs/reborn/memory-rd/README.md
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_memory/src/events.rs (1)
158-175: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a typed search-mode argument here.
Line 161 and Line 162 add adjacent booleans to the public event constructor. A provider can swap
full_textandvectorwhile still compiling, emitting wrong event/projection metadata. Pass a named contract type or enum instead.Refactor sketch
+pub struct MemorySearchModes { + pub full_text: bool, + pub vector: bool, +} + impl MemorySignificantEvent { pub fn search_performed( scope: &MemoryDocumentScope, source: MemorySignificantEventSource, - full_text: bool, - vector: bool, + modes: MemorySearchModes, result_count: u64, ) -> Self { Self { @@ - full_text: Some(full_text), - vector: Some(vector), + full_text: Some(modes.full_text), + vector: Some(modes.vector), audit_context: None, } }As per coding guidelines, use enums or strong types for known modes instead of booleans plus magic strings.
🤖 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/events.rs` around lines 158 - 175, The `search_performed` function currently accepts two adjacent boolean parameters `full_text` and `vector` which can be easily swapped by callers, resulting in incorrect event metadata. Replace these two boolean parameters with a single typed enum or named contract type that represents the search mode (for example, a `SearchMode` enum that captures the combination of search types). Update all call sites of `search_performed` to pass the appropriate enum variant instead of the individual boolean flags. This provides a strongly-typed contract that prevents accidental parameter swapping while making the caller's intent explicit.Source: Coding guidelines
crates/ironclaw_memory/src/metadata.rs (1)
31-40: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not warn-log metadata parse failures from the contract crate.
Line 35 emits
warn!with%errorfor memory metadata parsing. This is an internal diagnostic and can carry unredacted parser details into logs/TUI; keep the fallback if needed, but sanitize and downgrade it.Minimal fix
- Err(error) => { - tracing::warn!( - error = %error, - "failed to deserialize DocumentMetadata; falling back to defaults" - ); + Err(_) => { + tracing::debug!( + "failed to deserialize DocumentMetadata; falling back to defaults" + ); Self::default() }As per path instructions, REPL/TUI logging requires internal diagnostics to use
debug!, and as per coding guidelinesironclaw_memorymust not include unredacted user content in logs.🤖 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/metadata.rs` around lines 31 - 40, In the from_value method of DocumentMetadata, change the tracing::warn! call to tracing::debug! to downgrade the log level from warn to debug, and remove the error = %error parameter from the log to prevent unredacted parser details from being logged. Keep the descriptive fallback message and the Self::default() fallback behavior intact.Sources: Coding guidelines, Path instructions
crates/ironclaw_memory/src/path.rs (1)
240-244: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject reserved sidecar suffixes case-insensitively.
foo.META,data.CHUNKS, etc. bypass this check but collide with.meta/.chunks/.versionssidecars on case-insensitive filesystems.Suggested fix
for segment in value.split('/') { - if segment.ends_with(".meta") - || segment.ends_with(".chunks") - || segment.ends_with(".versions") + let segment_lower = segment.to_ascii_lowercase(); + if segment_lower.ends_with(".meta") + || segment_lower.ends_with(".chunks") + || segment_lower.ends_with(".versions") {Also extend
rejects_path_segments_ending_in_reserved_sidecar_suffixes()with mixed/uppercase cases.As per path instructions: “Path comparisons must be case-insensitive on macOS/Windows.”
🤖 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/path.rs` around lines 240 - 244, The suffix check for reserved sidecar files (.meta, .chunks, .versions) in the split('/') loop is performing case-sensitive comparisons, which allows uppercase or mixed-case versions like .META or .CHUNKS to bypass the check on case-insensitive filesystems. Modify the checks by converting the segment to lowercase before comparing against the reserved suffixes using to_lowercase() or similar. Additionally, extend the test function rejects_path_segments_ending_in_reserved_sidecar_suffixes() to include test cases with uppercase and mixed-case versions of these reserved suffixes to ensure the fix properly handles all cases.Source: Path instructions
♻️ Duplicate comments (2)
crates/ironclaw_host_runtime/src/memory_context.rs (1)
93-112: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse debug-level drops and preserve validation context.
These are internal admission diagnostics, and
.ok()?erases whyLoopSafeSummaryrejected provider-returned content.Proposed fix
- tracing::warn!("dropping memory context snippet with invalid ref"); + tracing::debug!("dropping memory context snippet with invalid ref"); return None; @@ - tracing::warn!("dropping memory context snippet without host-accepted wrapper"); + tracing::debug!("dropping memory context snippet without host-accepted wrapper"); return None; @@ - tracing::warn!("dropping oversized memory context snippet"); + tracing::debug!("dropping oversized memory context snippet"); return None; } - let safe_summary = LoopSafeSummary::new(snippet.safe_summary.clone()).ok()?; - let model_content = LoopSafeSummary::new(snippet.model_content).ok()?; + let safe_summary = match LoopSafeSummary::new(snippet.safe_summary.clone()) { + Ok(summary) => summary, + Err(error) => { + tracing::debug!(?error, "dropping memory context snippet with invalid safe summary"); + return None; + } + }; + let model_content = match LoopSafeSummary::new(snippet.model_content) { + Ok(content) => content, + Err(error) => { + tracing::debug!(?error, "dropping memory context snippet with invalid model content"); + return None; + } + }; @@ - tracing::warn!("dropping memory context snippet over aggregate budget"); + tracing::debug!("dropping memory context snippet over aggregate budget"); return None;As per path instructions, “REPL/TUI logging: info!/warn! corrupt the terminal UI — internal diagnostics use debug!” and “Fail loud: flag silent-failure patterns — .ok()? dropping errors...”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_host_runtime/src/memory_context.rs` around lines 93 - 112, Replace all tracing::warn! calls in this validation block with tracing::debug! since these are internal admission diagnostics that should not corrupt the REPL/TUI terminal. Additionally, replace the `.ok()?` error-erasing pattern on the LoopSafeSummary::new() calls with explicit error handling that captures and logs the actual validation failure reasons at debug level before returning None, so that silent failures are properly documented with their validation context rather than being silently dropped.Source: Path instructions
crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs (1)
33-35: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize
/memorybefore parsing profile input.Line 33 validates untrusted profile JSON before Line 35 checks the write grant, so an unauthorized caller can observe profile-field validation instead of failing closed with
FilesystemDenied.Proposed fix
- let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) - .map_err(map_memory_service_error)?; ensure_memory_mount(request, /* write */ true)?; + let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) + .map_err(map_memory_service_error)?;Add a caller-level regression for missing
/memorygrant plus invalid profile input. As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment...”🤖 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/first_party_tools/profile_set.rs` around lines 33 - 35, The authorization check via ensure_memory_mount is being performed after parsing the untrusted profile input through MemoryServiceProfileSetRequest::from_tool_input, which violates the fail-closed authorization principle. Move the ensure_memory_mount authorization check to execute before the MemoryServiceProfileSetRequest::from_tool_input parsing so that authorization is verified first, and unauthorized callers fail immediately with a FilesystemDenied error rather than potentially exposing validation error details about the profile structure.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/tests/memory_prompt_context.rs`:
- Around line 169-170: The test in memory_prompt_context.rs is not verifying the
context_profile_id field that was added to MemoryServiceContextRequest when
asserting on the captured request. Add an additional assert_eq statement after
the existing assertions on captured[0].1.query and captured[0].1.max_snippets to
verify that the context_profile_id field is correctly set in the captured
request, ensuring profile-routing behavior is tested and regressions are caught
at the caller level.
In `@crates/ironclaw_memory_native/Cargo.toml`:
- Line 5: The Cargo.toml file declares an invalid edition "2024" which does not
exist, and specifies rust-version "1.92" which is an unreleased version. Update
the edition field to a valid value (2015, 2018, or 2021) and change rust-version
to a stable released version of Rust. Apply these corrections to this file and
all other crates in the workspace that have the same issue, as the CI will fail
on cargo fmt and cargo clippy with these invalid values.
In `@crates/ironclaw_memory_native/src/backend.rs`:
- Around line 50-55: The public `with_prompt_write_safety_enforced` method on
`MemoryContext` allows any caller to set a bypass flag that causes backend write
paths to skip `enforce_prompt_write_safety()` checks, making this a convention
rather than an enforced boundary. Replace the public boolean mutator with an
internal capability token or newtype that can only be created and minted by the
filesystem adapter. Update the backend write paths that currently check for the
public flag to instead accept and validate this capability token, ensuring that
only the trusted filesystem adapter can construct the token and that the safety
boundary is enforced at construction time rather than as a convention.
In `@crates/ironclaw_memory_native/src/path.rs`:
- Around line 150-167: The MEMORY_BACKEND_DETAIL_MARKERS array is missing
patterns for common absolute paths that could expose system information to
users. Add the strings "/workspace/" and "/home/" to the
MEMORY_BACKEND_DETAIL_MARKERS constant array so that backend error messages
containing these absolute paths will be properly redacted when returned to
users, ensuring compliance with the coding guideline that prohibits exposing
absolute paths.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 77-94: The code currently silently falls back to default values
when fields have wrong types (e.g., append as string instead of bool, old_string
as non-string), which can change the write semantics unexpectedly. For the
append, old_string, and new_string fields extracted from the input object, add
explicit type validation that returns an error if these fields are present but
have incorrect types. Only use the default values when the fields are completely
absent from the input object. This ensures that malformed write options are
caught at the tool boundary rather than silently changing behavior.
- Around line 194-199: The MemoryServiceContextRequest struct currently uses a
raw String for context_profile_id, which violates the newtype pattern for
identifiers used elsewhere in the stack. Create a newtype wrapper for the
profile identifier (e.g., following the naming convention of other typed
identifiers in the codebase), define it in the memory contract module, and
replace the context_profile_id field's String type with this new typed
identifier newtype. This ensures type safety and consistency with how profile
binding IDs are modeled in adjacent layers.
---
Outside diff comments:
In `@crates/ironclaw_memory/src/events.rs`:
- Around line 158-175: The `search_performed` function currently accepts two
adjacent boolean parameters `full_text` and `vector` which can be easily swapped
by callers, resulting in incorrect event metadata. Replace these two boolean
parameters with a single typed enum or named contract type that represents the
search mode (for example, a `SearchMode` enum that captures the combination of
search types). Update all call sites of `search_performed` to pass the
appropriate enum variant instead of the individual boolean flags. This provides
a strongly-typed contract that prevents accidental parameter swapping while
making the caller's intent explicit.
In `@crates/ironclaw_memory/src/metadata.rs`:
- Around line 31-40: In the from_value method of DocumentMetadata, change the
tracing::warn! call to tracing::debug! to downgrade the log level from warn to
debug, and remove the error = %error parameter from the log to prevent
unredacted parser details from being logged. Keep the descriptive fallback
message and the Self::default() fallback behavior intact.
In `@crates/ironclaw_memory/src/path.rs`:
- Around line 240-244: The suffix check for reserved sidecar files (.meta,
.chunks, .versions) in the split('/') loop is performing case-sensitive
comparisons, which allows uppercase or mixed-case versions like .META or .CHUNKS
to bypass the check on case-insensitive filesystems. Modify the checks by
converting the segment to lowercase before comparing against the reserved
suffixes using to_lowercase() or similar. Additionally, extend the test function
rejects_path_segments_ending_in_reserved_sidecar_suffixes() to include test
cases with uppercase and mixed-case versions of these reserved suffixes to
ensure the fix properly handles all cases.
---
Duplicate comments:
In `@crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs`:
- Around line 33-35: The authorization check via ensure_memory_mount is being
performed after parsing the untrusted profile input through
MemoryServiceProfileSetRequest::from_tool_input, which violates the fail-closed
authorization principle. Move the ensure_memory_mount authorization check to
execute before the MemoryServiceProfileSetRequest::from_tool_input parsing so
that authorization is verified first, and unauthorized callers fail immediately
with a FilesystemDenied error rather than potentially exposing validation error
details about the profile structure.
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 93-112: Replace all tracing::warn! calls in this validation block
with tracing::debug! since these are internal admission diagnostics that should
not corrupt the REPL/TUI terminal. Additionally, replace the `.ok()?`
error-erasing pattern on the LoopSafeSummary::new() calls with explicit error
handling that captures and logs the actual validation failure reasons at debug
level before returning None, so that silent failures are properly documented
with their validation context rather than being silently dropped.
🪄 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: bc859006-4f06-45cd-988d-946a805b5e35
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (56)
Cargo.tomlcrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_extensions/src/v2.rscrates/ironclaw_extensions/tests/extension_contract.rscrates/ironclaw_extensions/tests/manifest_v2_contract.rscrates/ironclaw_host_api/src/host_port.rscrates/ironclaw_host_api/tests/host_api_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/extension_contracts.rscrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/memory_profile_binding.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/host_api_contract_composition.rscrates/ironclaw_host_runtime/tests/memory_profile_binding.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_host_runtime/tests/tool_surface_contract.rscrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rs
BenKurrek
commented
Jun 23, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
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_native/src/service.rs (1)
343-348: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply the protected-document filter to
retrieve_contexttoo.
searchandtreenow dropHEARTBEAT.md/BOOTSTRAP.md, butretrieve_contextstill only retains matching scope and finite scores. A buggy/stale index result for a protected document would be turned into a model-visible safe summary. As per coding guidelines, protected/internal memory documents must not be exposed across public/model surfaces.Suggested patch
- results.retain(|result| result.path.scope() == context.scope() && result.score.is_finite()); + results.retain(|result| { + result.path.scope() == context.scope() + && result.score.is_finite() + && !is_protected_memory_path(result.path.relative_path()) + });🤖 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_native/src/service.rs` around lines 343 - 348, The retrieve_context method in the service.rs file currently filters results only by scope and finite scores, but does not exclude protected documents like HEARTBEAT.md and BOOTSTRAP.md. Update the results.retain closure to add an additional condition that filters out protected documents, similar to the filtering already applied in the search and tree methods. This ensures protected and internal memory documents are not exposed through the retrieve_context public surface to models or external consumers.Source: Coding guidelines
♻️ Duplicate comments (1)
crates/ironclaw_memory_native/src/path.rs (1)
166-167: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd a regression test for the new absolute-path redaction markers.
These markers close a host-path disclosure gap, but the changed tests shown for this file cover path validation rather than
sanitize_memory_backend_reason. Add a same-module test for/workspace/and/home/so the privacy fix is pinned. As per coding guidelines, “Every bug fix must include a regression test.”Suggested test shape
+#[test] +fn sanitizes_common_host_absolute_paths_in_backend_reasons() { + assert_eq!( + sanitize_memory_backend_reason("failed to open /workspace/project/memory.db"), + "memory backend operation failed" + ); + assert_eq!( + sanitize_memory_backend_reason("failed to open /home/alice/.cache/memory.db"), + "memory backend operation failed" + ); +}🤖 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_native/src/path.rs` around lines 166 - 167, The new absolute-path redaction markers for `/workspace/` and `/home/` need a regression test to ensure the privacy fix is maintained. Add a same-module test function in the path.rs file that specifically tests the `sanitize_memory_backend_reason` function with inputs containing `/workspace/` and `/home/` paths, verifying that these paths are properly redacted in the output. This test should validate that the host-path disclosure vulnerability is actually closed by the redaction markers.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/src/metadata.rs`:
- Around line 46-48: The logging level for the DocumentMetadata deserialization
fallback error is incorrect. Change the tracing::warn! call that logs "failed to
deserialize persisted DocumentMetadata; falling back to defaults" to use
tracing::debug! instead, as this is an internal diagnostic that should not
appear in the REPL/TUI display. Keep the same error context and message content
when making this change.
---
Outside diff comments:
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 343-348: The retrieve_context method in the service.rs file
currently filters results only by scope and finite scores, but does not exclude
protected documents like HEARTBEAT.md and BOOTSTRAP.md. Update the
results.retain closure to add an additional condition that filters out protected
documents, similar to the filtering already applied in the search and tree
methods. This ensures protected and internal memory documents are not exposed
through the retrieve_context public surface to models or external consumers.
---
Duplicate comments:
In `@crates/ironclaw_memory_native/src/path.rs`:
- Around line 166-167: The new absolute-path redaction markers for `/workspace/`
and `/home/` need a regression test to ensure the privacy fix is maintained. Add
a same-module test function in the path.rs file that specifically tests the
`sanitize_memory_backend_reason` function with inputs containing `/workspace/`
and `/home/` paths, verifying that these paths are properly redacted in the
output. This test should validate that the host-path disclosure vulnerability is
actually closed by the redaction markers.
🪄 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: d4a7650f-2384-47eb-a538-c2267b85793c
📒 Files selected for processing (6)
crates/ironclaw_host_runtime/src/memory_profile_binding.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/tests/memory_service_facade.rs
BenKurrek
added a commit
that referenced
this pull request
Jun 23, 2026
…build path Stacked on #5163 (reborn/memory-placement-m2-lift). #5163 made the memory layer a provider-neutral `MemoryService` (agnostic/native crate split) but exposed no public way for whoever builds the runtime to select the provider — only a test-only `MemoryCapabilityState::with_memory_service_for_test`. This adds the general provider seam #5163 is built for: the embedder selects the runtime's `MemoryService` at build time, and ironclaw uses it the same uniform way regardless of which provider it is. Native stays the default. - `ironclaw_host_runtime`: - `MemoryCapabilityState` carries the composed `memory_service` (renamed from any "override" concept); `service_for` is the single, uniform selection seam — composed provider if present, else the default native provider built from the request filesystem. - `BuiltinFirstPartyTools::with_memory_service(..)` and the public registry builders `builtin_first_party_handlers_with_trigger_create_hook_and_memory_service(..)` (+ the `_for_process_backend` variant for the production path) compose the built-in memory capabilities with the selected provider. `None` selects native and is byte-for-byte equivalent to the existing builders. - Re-export `ironclaw_memory::MemoryService` so composition callers can name the trait without a direct dependency. - `ironclaw_reborn_composition`: - `RebornBuildInput::with_memory_service(Arc<dyn MemoryService>)` selects the provider, threaded through BOTH the local-dev/hosted-single-tenant path (`build_local_runtime`) and the production path (`build_production_shaped` -> `RebornProductionBuildContext` -> `build_backend_production`) so the seam works uniformly on every profile. No "override" / escape-hatch concept and no special-casing of any provider. ironclaw has zero awareness of pre-seeding: a provider self-seeds in its own constructor and is supplied as a plain `Arc<dyn MemoryService>`. Default (no provider supplied = native) behavior is unchanged on all paths. Design note: #5163's `MemoryProfileBinding` / `resolve_memory_profile_bindings` is a contract-only resolver (its own docs: "does not dispatch calls or change the existing memory runtime path") with zero runtime consumers and no ExtensionId->MemoryService registry — wiring that into dispatch is the deferred capability-surface surgery. So per the latitude given, this lands the general, default-native provider parameter on the public build input, routed through the single uniform `service_for` selection seam (not a side-channel beside it), working on every path. Tests: provider dispatch routes through the composed provider; build-input records/defaults the provider. Builds clean under default, libsql, and postgres features; clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BenKurrek
added a commit
that referenced
this pull request
Jun 23, 2026
… path Stacked on #5163 (reborn/memory-placement-m2-lift). Expands the NATIVE memory provider so it can optionally be initialized with a set of starting documents, exposed on the public composition build path. This is a general capability (tests / demos / migrations), not benchmark-specific. - `ironclaw_memory_native`: - New `SeedMemoryDocument { tenant_id, user_id, agent_id, project_id, path, content, metadata }` — a starting document with its own scope. - `NativeMemoryService::seed_documents(docs)` writes each seed through the REAL native write path by calling `MemoryService::write` — the exact entrypoint an agent write uses. Seeds are therefore stored, versioned, prompt-safety-scanned, and chunked/indexed identically to agent writes. Per-doc scope is honored via a `MemoryInvocation` built from each seed's tenant/user/agent/project (the native path derives the `MemoryContext` exactly as for an agent write). Nothing is special-cased or bypassed. - `ironclaw_reborn_composition`: - `RebornBuildInput::with_seed_memory(impl IntoIterator<Item = SeedMemoryDocument>)`. - Threaded into the factory's native construction on all paths: local-dev / hosted-single-tenant (`build_local_runtime`) and production libsql/postgres (`build_production_shaped` -> `RebornProductionBuildContext` -> `build_backend_production`). Seeds are written over the same composed filesystem the runtime's memory capability reads, before the runtime is returned. Empty (the default) writes nothing. Search, capabilities, and backend behavior are UNTOUCHED: `search.rs`, `backend.rs`, `indexer.rs`, the `MemoryBackendCapabilities`, and the whole `ironclaw_memory` crate have zero changes. An in-memory backend still has no full-text search, so search behavior is unchanged. Default (no seed) is byte-identical to today on every path. Drops the earlier `with_memory_service` provider-swap (would have allowed a custom-search provider — the opposite of using ironclaw's real native memory). Tests: seeds write through the native path and are readable/listed (native crate); end-to-end through the real runtime memory dispatch and per-doc scope honored; default path yields an empty memory tree. Builds clean under default, libsql, and postgres features; clippy clean (the one `secret_store` warning pre-exists on the base branch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BenKurrek
changed the title
BenKurrek
force-pushed
the
reborn/memory-placement-m2-lift
branch
from
June 23, 2026 22:17
2cec1be to
fe97aff
Compare
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
BenKurrek
commented
Jun 23, 2026
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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_native/src/backend.rs`:
- Around line 50-57: The `MemoryBackendWriteOptions` struct currently exposes
the `prompt_safety_already_enforced` field as public, allowing any caller to
bypass the prompt safety enforcement check. Make the
`prompt_safety_already_enforced` field private and create an opaque/unforgeable
capability mechanism (such as a private token type or sealed construction
method) that only the filesystem adapter can use to set this flag after
successful enforcement. This ensures the guarantee stated in the comment that
foreign crates cannot forge the already-enforced marker. Apply this same pattern
consistently across all related locations where `MemoryBackendWriteOptions` is
defined or used (around lines 437-455, 551-569, 649-667, and 759-777) to
maintain the security contract across all backend implementations.
In `@crates/ironclaw_memory_native/src/safety.rs`:
- Around line 405-412: The `record_prompt_write_safety_event` call is a boundary
sink and its raw error output is being logged directly via `error = %error` in
the tracing debug statement, which can leak sensitive information like backend
paths or secrets. Replace the raw error logging with a sanitized, stable error
code instead of displaying the actual error details. Keep the descriptive
context in the log message but remove or replace the `error = %error` field with
a stable identifier that doesn't expose internal implementation details.
In `@crates/ironclaw_memory_native/tests/memory_service_facade.rs`:
- Around line 33-95: Add two new regression tests to validate the security
filtering behaviors in the NativeMemoryService facade methods. Create a test
that exercises the search method to verify protected documents are suppressed
from results, and create another test for the tree method to verify scope
isolation filtering is enforced. These tests should be separate from the
existing native_provider_reads_writes_lists_and_searches_through_memory_service
integration test and should explicitly validate that the defense-in-depth
filtering invariants (scope isolation and protected-document suppression) cannot
regress.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 213-216: The MemoryServiceProfileSetResponse struct uses a
free-form String type for the status field instead of leveraging a
strongly-typed enum, despite the contract only emitting "ok". Create a new enum
type called MemoryProfileSetStatus with an Ok variant to represent the known
status values, then update the status field in MemoryServiceProfileSetResponse
to use MemoryProfileSetStatus instead of String. Finally, ensure that any code
producing this response (in the native producer) returns
MemoryProfileSetStatus::Ok to match the contract expectations and provide strong
typing for control flow.
- Around line 184-190: The from_tool_input method currently silently converts
non-string path values to an empty string, masking user errors. Instead,
distinguish between missing/null paths (which should default to root empty
string) and present non-string paths (which should return an error). Check if
the path key exists in the input object, and only if it exists, validate that
its value is actually a string type before accepting it. If path is present but
not a string, return a MemoryServiceError indicating the invalid type.
🪄 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: ec0288ee-40ec-4dca-a00e-48ecd567a7ad
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (49)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_memory/AGENTS.mdcrates/ironclaw_memory/CLAUDE.mdcrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rscrates/ironclaw_product_adapters/tests/product_adapter_contract.rs
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
crates/ironclaw_memory_native/src/service.rs (1)
49-51: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFinish protected memory-path redaction at every public DTO boundary.
is_protected_memory_path()still only matches two exact root filenames, whilereadcan returnBOOTSTRAP.md/HEARTBEAT.mddirectly andwrite("heartbeat")returnsHEARTBEAT.mdat Line 221. Also cover.system/andAGENTS.md, use case-insensitive comparisons, and reject or alias protected paths before returning user/tool DTOs. This is the same protected-name invariant as the earlier thread, but these call sites remain exposed.As per coding guidelines, “Never expose to users … internal file names (
.system/,AGENTS.md,HEARTBEAT.md,BOOTSTRAP.md).”Suggested direction
fn is_protected_memory_path(relative_path: &str) -> bool { - relative_path == HEARTBEAT_PATH || relative_path == BOOTSTRAP_PATH + let normalized = relative_path.trim_start_matches('/'); + let lower = normalized.to_ascii_lowercase(); + let leaf = lower.rsplit('/').next().unwrap_or(lower.as_str()); + + leaf == "heartbeat.md" + || leaf == "bootstrap.md" + || leaf == "agents.md" + || lower == ".system" + || lower.starts_with(".system/") }Then use the same predicate in
read, direct-path writes, and response path mapping for protected aliases.Also applies to: 219-247
🤖 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_native/src/service.rs` around lines 49 - 51, The `is_protected_memory_path()` function is incomplete and only matches two exact root filenames, but internal paths like BOOTSTRAP.md, HEARTBEAT.md, AGENTS.md, and .system/ directory can still be exposed to users through the read, write, and response functions. Expand the `is_protected_memory_path()` function to perform case-insensitive matching for all protected internal file names and directory patterns (HEARTBEAT_PATH, BOOTSTRAP_PATH, AGENTS.md, and .system/), then apply this predicate consistently at all public DTO boundaries including the read function, direct-path writes around line 221, and response path mapping to ensure protected paths are rejected or aliased before returning user/tool DTOs.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/first_party_tools/memory.rs`:
- Around line 253-256: The emit_audit error handling at line 256 converts the
raw error to a string using error.to_string(), which can expose sensitive
information like host paths or backend details. Replace the raw error
stringification in MemoryEventSinkError::new() with a sanitized error message
that includes only safe information such as error kind or correlation
identifiers, avoiding any raw backend error details. Apply the same sanitization
pattern to the error logging in the MemoryServiceError handling around lines
362-365 where the raw source is being propagated.
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 116-122: The filter closure in the search function currently
validates scope and protected paths, but does not check for non-finite scores.
Add an additional condition to the filter in the search function that verifies
result scores are finite (not NaN or infinity), similar to the validation
performed in retrieve_context at lines 359-363. This will prevent non-finite
scores from buggy backends from reaching downstream JSON/tool serialization.
- Around line 304-313: The code validates existing profile document fields
(timezone, locale, location) to ensure they are strings, but then immediately
inserts request.fields into the document without validating whether those
incoming fields contain non-string values for the known profile fields. Add a
validation loop before the doc.insert(...) call that checks if any of the known
fields (timezone, locale, location) present in request.fields are non-strings,
and return MemoryServiceError::operation() if validation fails, similar to the
existing profile validation loop.
In `@crates/ironclaw_memory/AGENTS.md`:
- Around line 6-7: The dependency guardrail wording in AGENTS.md at lines 6 and
24 is too restrictive and conflicts with the crate's actual use of third-party
dependencies. Update the wording to clarify that the guardrail rule applies only
to internal IronClaw crate dependencies (like `ironclaw_host_api`), not to
third-party crates such as `serde`, `serde_json`, `async-trait`, and `chrono_tz`
which are legitimately used in the contract layer. Scope the rule to make clear
that third-party dependencies are acceptable when necessary.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 35-36: The optional_u64 function at line 461 silently converts
type errors to None, causing malformed numeric input to fall back to defaults
instead of failing. Refactor optional_u64 to return a Result that distinguishes
between missing fields (Ok(None)) and present-but-wrong-typed values (Err), then
update the callers at line 35 (limit) and line 190 (depth) to use the ? operator
to propagate these errors as input validation failures. This ensures that when a
numeric field like "limit" or "depth" is present in the input, any type mismatch
is treated as an error rather than silently defaulting to a permitted value.
---
Duplicate comments:
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 49-51: The `is_protected_memory_path()` function is incomplete and
only matches two exact root filenames, but internal paths like BOOTSTRAP.md,
HEARTBEAT.md, AGENTS.md, and .system/ directory can still be exposed to users
through the read, write, and response functions. Expand the
`is_protected_memory_path()` function to perform case-insensitive matching for
all protected internal file names and directory patterns (HEARTBEAT_PATH,
BOOTSTRAP_PATH, AGENTS.md, and .system/), then apply this predicate consistently
at all public DTO boundaries including the read function, direct-path writes
around line 221, and response path mapping to ensure protected paths are
rejected or aliased before returning user/tool DTOs.
🪄 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: 0402961a-e76c-47d8-a243-3c13667f76d1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (49)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_memory/AGENTS.mdcrates/ironclaw_memory/CLAUDE.mdcrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rscrates/ironclaw_product_adapters/tests/product_adapter_contract.rs
BenKurrek
force-pushed
the
reborn/memory-placement-m2-lift
branch
from
June 24, 2026 02:24
fe97aff to
aaf6470
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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/src/path.rs (1)
248-252: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHarden reserved-suffix rejection to be case-insensitive.
At Line 248, suffix checks are case-sensitive, so
foo.META/foo.Chunkscan bypass validation and collide with sidecar namespaces on case-insensitive filesystems.Suggested fix
- for segment in value.split('/') { - if segment.ends_with(".meta") - || segment.ends_with(".chunks") - || segment.ends_with(".versions") + for segment in value.split('/') { + let segment = segment.to_ascii_lowercase(); + if segment.ends_with(".meta") + || segment.ends_with(".chunks") + || segment.ends_with(".versions") { return Err(HostApiError::InvalidPath {As per path instructions, path comparisons for user-supplied paths must be case-insensitive on macOS/Windows.
🤖 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/path.rs` around lines 248 - 252, The reserved suffix checks in the for loop that iterates through segments are case-sensitive when using ends_with() on the segment variable, allowing uppercase variants like ".META" or ".CHUNKS" to bypass validation. Convert the segment to lowercase before performing the ends_with() checks for ".meta", ".chunks", and ".versions" to ensure the validation works correctly on case-insensitive filesystems.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.
Inline comments:
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 23-26: Add a byte size limit constant for the snippet_ref to cap
its size before any other validation occurs. In the validation logic around
lines 88-99 that checks snippet_ref (specifically where the
MEMORY_CONTEXT_REF_PREFIX is verified and character scans happen), add an
upfront length check that rejects any snippet_ref exceeding the new byte limit
before allowing it to proceed to prefix checks or enter LoopContextSnippet.
Additionally, create a caller-level test that verifies oversized snippet_ref
values are properly rejected and cannot bypass the byte budget constraints.
In `@crates/ironclaw_memory/Cargo.toml`:
- Around line 14-26: Remove all non-host-api dependencies (async-trait,
chrono-tz, serde, serde_json, sha2, and tracing) from the dependencies section
of the Cargo.toml, keeping only the ironclaw_host_api dependency. Extract the
timezone validation logic currently in
MemoryServiceProfileSetRequest::from_tool_input that relies on chrono-tz and
move it into a provider crate or hoist the necessary functionality into
ironclaw_host_api if it is part of the contract surface. Similarly, extract the
diagnostic logging logic from DocumentMetadata::from_value that uses tracing and
relocate it to provider crates or add it to ironclaw_host_api if required by the
contract. Ensure all functionality is preserved by updating the call sites to
use the new locations of this logic.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 118-124: The documentation comment describing memory
write-operation outcomes with statuses like "cleared", "written", and "patched"
is incorrectly placed above the MemoryProfileSetStatus type definition, but this
type actually represents the outcome of a profile_set operation (which only
reports success as "ok"). Move the write-operation status documentation to the
correct type it describes (likely an enum with cleared, written, and patched
variants), and replace it with documentation for MemoryProfileSetStatus that
accurately describes its purpose as tracking the status of profile_set
operations where the native provider only reports success cases and errors
surface as exceptions.
---
Outside diff comments:
In `@crates/ironclaw_memory/src/path.rs`:
- Around line 248-252: The reserved suffix checks in the for loop that iterates
through segments are case-sensitive when using ends_with() on the segment
variable, allowing uppercase variants like ".META" or ".CHUNKS" to bypass
validation. Convert the segment to lowercase before performing the ends_with()
checks for ".meta", ".chunks", and ".versions" to ensure the validation works
correctly on case-insensitive filesystems.
🪄 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: ea0a72a6-9a94-4068-95a4-7722ebe1a7f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (49)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_memory/AGENTS.mdcrates/ironclaw_memory/CLAUDE.mdcrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rscrates/ironclaw_product_adapters/tests/product_adapter_contract.rs
BenKurrek
force-pushed
the
reborn/memory-placement-m2-lift
branch
from
June 24, 2026 02:54
aaf6470 to
3ce1649
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
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/src/path.rs (1)
248-252: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize reserved-sidecar suffix checks before comparison.
Line 248 compares suffixes case-sensitively. On case-insensitive filesystems, names like
foo.METAcan aliasfoo.meta, which can re-open sidecar/document collisions the validator is meant to block.Suggested fix
- for segment in value.split('/') { - if segment.ends_with(".meta") - || segment.ends_with(".chunks") - || segment.ends_with(".versions") + for segment in value.split('/') { + let segment_lower = segment.to_ascii_lowercase(); + if segment_lower.ends_with(".meta") + || segment_lower.ends_with(".chunks") + || segment_lower.ends_with(".versions") { return Err(HostApiError::InvalidPath { value,As per coding guidelines, “When comparing user-supplied strings (file paths, media types, extension names), normalize to lowercase with
.to_ascii_lowercase()… Path comparisons must be case-insensitive on macOS/Windows.”🤖 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/path.rs` around lines 248 - 252, The suffix checks for reserved sidecar files in the loop that iterates over segments from value.split('/') are performing case-sensitive comparisons with ends_with(".meta"), ends_with(".chunks"), and ends_with(".versions"). On case-insensitive filesystems, this allows filenames like "foo.META" to bypass the validation. Normalize the segment to lowercase using to_ascii_lowercase() before performing the ends_with checks so that case-insensitive filesystems properly detect all variants of reserved suffixes.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_architecture/tests/reborn_dependency_boundaries.rs`:
- Around line 2079-2084: The new boundary rule being added for
ironclaw_memory_native at line 2079 introduces a provider crate that must be
protected by the same layer boundary rules that currently restrict
ironclaw_memory. Search through the boundary rules test file for all existing
BoundaryRule entries that include ironclaw_memory in their forbidden_by lists or
dependency restrictions, and add equivalent restrictions for
ironclaw_memory_native to each of those rules. This ensures that the same crates
cannot bypass the boundary intent by importing through the native provider
instead of the abstract memory crate.
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 52-79: The current code allows the provider-returned context
candidates to be unbounded, potentially inspecting/logging many invalid snippets
if a provider returns them. Add a host-owned candidate cap constant and pass a
capped max_snippets value to the retrieve_context call via the
MemoryServiceContextRequest (use a reasonable limit like 2-3 times the requested
max_snippets to allow for invalid snippets). This bounds the number of
candidates the provider can return before the admission filtering loop in the
logic following the retrieve_context call. Additionally, add a test case that
verifies this behavior by having a provider return many invalid snippets
followed by valid ones, ensuring the host properly limits inspection to the
candidate cap.
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 48-52: Extend the is_protected_memory_path() function to
comprehensively filter all protected internal paths case-insensitively,
including HEARTBEAT_PATH, BOOTSTRAP_PATH, AGENTS.md, and any .system/ prefixed
paths. This function must be applied consistently across all search, tree,
retrieve_context, and write operations (referenced at lines 117-130, 222-250,
271-278, 362-365) before returning DTOs to ensure internal file names never leak
to users. Additionally, when the write operation returns a response for the
heartbeat alias, map the response path back from HEARTBEAT.md to the heartbeat
alias instead of exposing the internal file name.
In `@crates/ironclaw_memory_native/tests/memory_service_facade.rs`:
- Around line 98-181: Add regression test coverage for non-finite scores (NaN
and INFINITY) in the
native_search_drops_out_of_scope_and_protected_system_documents test. In the
MockSearchBackend results vector, add one or more search_result entries with
non-finite score values (such as f64::NAN or f64::INFINITY) that should be
filtered out by NativeMemoryService::search alongside the existing scope and
protected-path filtering tests. Ensure the final assertion still expects only
one result (the in-scope, non-protected, finite-score document) to pass, which
will verify the non-finite score filter is working and prevent regression.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 167-169: The condition checking `list_versions` in the input
validation block currently only validates that a boolean value equals true,
which causes malformed values (such as strings or numbers) to be silently
ignored instead of being rejected. Modify the validation logic to explicitly
check if the `list_versions` key is present in the input, and if it is present,
ensure it is actually a boolean using `Value::as_bool()`. If `list_versions` is
present but not a boolean value, return an error using the error handling
pattern with `?` operator rather than silently proceeding, so that boundary
input errors are properly flagged and propagated as per the fail-loud guidance.
---
Outside diff comments:
In `@crates/ironclaw_memory/src/path.rs`:
- Around line 248-252: The suffix checks for reserved sidecar files in the loop
that iterates over segments from value.split('/') are performing case-sensitive
comparisons with ends_with(".meta"), ends_with(".chunks"), and
ends_with(".versions"). On case-insensitive filesystems, this allows filenames
like "foo.META" to bypass the validation. Normalize the segment to lowercase
using to_ascii_lowercase() before performing the ends_with checks so that
case-insensitive filesystems properly detect all variants of reserved suffixes.
🪄 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: a6369ad0-23c3-4c9b-8dff-5f49cec242f9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (49)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_memory/AGENTS.mdcrates/ironclaw_memory/CLAUDE.mdcrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rscrates/ironclaw_product_adapters/tests/product_adapter_contract.rs
serrrfirat
reviewed
Jun 24, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Collaborator
There was a problem hiding this comment.
Multi-agent review plus thermonuclear maintainability pass for nearai/ironclaw#5163 at 3ce1649c30d53109bba53f99f90e4738f1bd94d6.
Reviewers run:
- Security: 1 finding
- Bugs: 1 finding
- Performance/Concurrency: clean
- Tests: 6 coverage gaps, summarized below
- Conventions: 3 findings
- Thermonuclear maintainability: structural issues folded into the inline review
Main findings are inline. Summary:
- Disabled memory context profiles moved from a host-enforced no-call guard to provider-side behavior. Since
ProductionMemoryPromptContextServiceaccepts anyArc<dyn MemoryService>, the host should still fail closed before calling the provider. memory-snippet:*refs are behaviorally changed by a new length-prefixed FNV layout instead of the existing shared trailing-separator helper.- The provider split updates crate-local ownership docs but leaves the Reborn memory/storage contracts saying concrete search/indexing/prompt-context behavior still lives in
ironclaw_memory. - A prompt-safety bypass comment promises more than the code enforces: the public agnostic
MemoryContextstill carries an allowance setter/read path. - Profile read/write ownership is now split: the read side constructs the native repository directly while the write side has a private duplicate scope/path helper in
NativeMemoryService.
Test gaps worth addressing with the fixes:
- Restore caller-level host test coverage for disabled memory profiles returning empty without calling the memory service.
- Add provider tests for
retrieve_contextdropping protected documents and forsearchdropping non-finite scores before tool serialization. - Add caller-level
memory_writemalformed-field tests for wrong-typedcontent,append,old_string,new_string,replace_all, andmetadata. - Cover empty
new_stringpatch rejection through the first-party memory write path. - Add a small
MemoryServiceErrortest provingoperation_from/unavailable_frompreservesource()whileDisplayremains sanitized.
Because all findings are Medium severity, this review is posted as COMMENT rather than REQUEST_CHANGES.
BenKurrek
force-pushed
the
reborn/memory-placement-m2-lift
branch
from
June 24, 2026 12:46
3ce1649 to
2c5ed5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_memory/src/events.rs (1)
158-175: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace the positional search-mode booleans with a typed mode value.
search_performed(scope, source, full_text, vector, ...)makes two adjacentbools part of a cross-crate contract. A call-site swap still compiles and silently mislabels telemetry. Prefer a small mode struct/enum here and incrates/ironclaw_memory_native/src/backend.rs:913-928.As per coding guidelines,
**/*.rs: “Use enums for units, shapes, and modes ... instead of booleans plus magic strings.”🤖 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/events.rs` around lines 158 - 175, The search telemetry API currently uses two adjacent bools in MemorySignificantEvent::search_performed, which makes the mode contract easy to misuse; replace full_text/vector with a typed search mode value (small enum or struct) and thread that through the call site in backend.rs around the search event creation. Update both the event constructor and its caller so the mode is explicit and impossible to swap accidentally, while keeping the result_count and other fields unchanged.Source: Coding guidelines
crates/ironclaw_memory_native/tests/memory_backend_contract.rs (1)
243-308: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the mirrored
append_fileregression.Lines 243-308 prove the new
prompt_safety_already_enforcedhandoff forwrite_file, but the same fix was added separately incrates/ironclaw_memory_native/src/filesystem.rsLines 386-400 forappend_file. That append path has its own retry loop and backend call, so it can regress independently and reopen the adapter/backend double-check bug on protected appends.As per coding guidelines, “Every bug fix must include a regression test (
#[test]or#[tokio::test])”, and as per path instructions, “Test through the caller”.🤖 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_native/tests/memory_backend_contract.rs` around lines 243 - 308, Add a mirrored regression test for append_file that exercises the same adapter/backend prompt-safety handoff as backend_does_not_re_reject_adapter_approved_warn_or_bypass. Use MemoryBackendFilesystemAdapter, RepositoryMemoryBackend, and PromptWriteSafetyPolicy to verify an adapter-approved append is not re-evaluated by the backend when prompt_safety_already_enforced should be set. Make the backend use an AlwaysRejectIfRecheckedPolicy and assert the append succeeds through the caller path, covering the append_file retry/backend branch separately from write_file.Sources: Coding guidelines, Path instructions
♻️ Duplicate comments (7)
crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs (1)
2079-2084: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMirror
ironclaw_memory_nativeforbids whereverironclaw_memoryis forbiddenLine 2079 adds the native-provider rule, but many existing rules still block only
ironclaw_memory(for example Line 1665, Line 1720, Line 2231), leaving a dependency bypass path throughironclaw_memory_native.Suggested guardrail test
+#[test] +fn crates_forbidding_contract_memory_also_forbid_native_memory_provider() { + const EXEMPT: &[&str] = &[]; + for rule in boundary_rules() { + let blocks_contract = rule.forbidden.contains(&"ironclaw_memory"); + let blocks_native = rule.forbidden.contains(&"ironclaw_memory_native"); + if blocks_contract && !EXEMPT.contains(&rule.crate_name) { + assert!( + blocks_native, + "{} forbids ironclaw_memory but not ironclaw_memory_native", + rule.crate_name + ); + } + } +}As per path instructions, “Module specs win ties … flag diffs that contradict their module spec”; allowing a native-provider bypass contradicts this boundary rule intent.
🤖 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 2079 - 2084, Add the same boundary restrictions to ironclaw_memory_native wherever the corresponding ironclaw_memory rule is currently blocked, since the native provider can otherwise bypass the intended module boundary. Update the relevant BoundaryRule entries in reborn_dependency_boundaries.rs so the denylist/forbidden targets mirror between ironclaw_memory and ironclaw_memory_native, using the existing BoundaryRule patterns around the affected crate_name checks. Ensure any guardrail coverage also asserts the native provider cannot depend on host composition, dispatch, or higher-authority subsystems when ironclaw_memory is forbidden.Source: Path instructions
crates/ironclaw_host_runtime/src/memory_context.rs (2)
90-98: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNormalize disabled context aliases at the host gate.
This check is supposed to avoid any provider call, but exact matching fails open for case or whitespace variants like
Memory-Disabled.As per coding guidelines, “When comparing user-supplied strings … normalize to lowercase with
.to_ascii_lowercase(),” and as per path instructions, “Fail closed…”Suggested fix
- MEMORY_DISABLED_ALIASES.contains(&context_profile_id) + MEMORY_DISABLED_ALIASES + .iter() + .any(|alias| context_profile_id.trim().eq_ignore_ascii_case(alias))🤖 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_context.rs` around lines 90 - 98, The memory_context_disabled gate is doing an exact alias match, so case or whitespace variants like Memory-Disabled can slip through and still reach provider calls. Update memory_context_disabled to normalize the incoming context_profile_id before checking MEMORY_DISABLED_ALIASES by trimming whitespace and converting to lowercase (using to_ascii_lowercase), then compare against the normalized aliases so the gate fails closed. Keep the fix localized to memory_context_disabled and its alias list in memory_context.rs.Sources: Coding guidelines, Path instructions
23-26: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound provider-controlled context candidates and refs.
max_snippetsis forwarded unbounded, invalid snippets do not incrementadmitted.len(), andsnippet_refhas no byte cap. A provider can force excessive admission work or pass a huge ref outside the content budgets.As per coding guidelines, “Keep runtime crates untrusted; host-runtime must mediate secrets/network/redaction/accounting.”
Suggested guard shape
const MAX_MEMORY_CONTEXT_SNIPPET_BYTES: usize = 512; const MAX_MEMORY_CONTEXT_TOTAL_BYTES: usize = 4 * 1024; +const MAX_MEMORY_CONTEXT_REF_BYTES: usize = 256; +const MAX_MEMORY_CONTEXT_CANDIDATE_SNIPPETS: usize = 64; const MEMORY_CONTEXT_REF_PREFIX: &str = "memory-snippet:"; @@ + let requested_snippets = request + .max_snippets + .min(MAX_MEMORY_CONTEXT_CANDIDATE_SNIPPETS); let snippets = self .memory_service .retrieve_context( invocation, MemoryServiceContextRequest { query: request.query, - max_snippets: request.max_snippets, + max_snippets: requested_snippets, context_profile_id, }, @@ - for snippet in snippets { - if admitted.len() >= request.max_snippets { + for snippet in snippets.into_iter().take(MAX_MEMORY_CONTEXT_CANDIDATE_SNIPPETS) { + if admitted.len() >= requested_snippets { break; } @@ - if !snippet.snippet_ref.starts_with(MEMORY_CONTEXT_REF_PREFIX) + if snippet.snippet_ref.len() > MAX_MEMORY_CONTEXT_REF_BYTES + || !snippet.snippet_ref.starts_with(MEMORY_CONTEXT_REF_PREFIX)Also applies to: 62-78, 100-115
🤖 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_context.rs` around lines 23 - 26, The memory admission path in memory_context.rs is accepting provider-controlled inputs without enough bounds: cap the forwarded max_snippets, ensure invalid snippets still count toward the admission limit so admitted.len() cannot be bypassed, and enforce a byte limit on snippet_ref before it is accepted. Update the logic around the memory snippet admission flow and the MEMORY_CONTEXT_REF_PREFIX handling to reject oversized refs and stop untrusted providers from forcing excessive work or exceeding the content budget.Source: Coding guidelines
crates/ironclaw_memory_native/tests/memory_service_facade.rs (1)
97-268: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd caller-level coverage for
search/treeoutput filters.The suite only locks
retrieve_context; add tests that driveNativeMemoryService::searchandNativeMemoryService::treewith cross-scope, protected-path, and non-finite-score backend results so the public DTO filters cannot regress.As per coding guidelines, “Every bug fix must include a regression test,” and as per path instructions, “Test through the caller.”
Also applies to: 507-539
🤖 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_native/tests/memory_service_facade.rs` around lines 97 - 268, Add caller-level regression tests for the public NativeMemoryService APIs, not just retrieve_context: exercise search and tree with mocked backend results that include cross-scope entries, protected-path entries, and non-finite scores so the DTO filtering is verified through the caller. Reuse the existing NativeMemoryService, MockSearchBackend, search_result/search_result_with_agent, and invocation helpers to assert only valid in-scope, finite results survive in search and tree output, matching the filtering already covered in retrieve_context.Sources: Coding guidelines, Path instructions
crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs (1)
33-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize
/memorybefore parsing profile input.Current ordering validates untrusted fields before the write grant, so unauthorized callers can receive validation feedback instead of failing closed with
FilesystemDenied.As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment…”
Suggested fix
- // Validate the profile fields first, then authorize the `/memory` write — - // matching the pre-lift ordering (`validated_fields` ran before - // `ensure_memory_mount`). - let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) - .map_err(map_memory_service_error)?; ensure_memory_mount(request, /* write */ true)?; + let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) + .map_err(map_memory_service_error)?;🤖 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/first_party_tools/profile_set.rs` around lines 33 - 38, The `/memory` write check in profile_set is happening after untrusted input parsing, so move `ensure_memory_mount(request, /* write */ true)?` to run before `MemoryServiceProfileSetRequest::from_tool_input(&request.input)` and keep the request rejected with `FilesystemDenied` before any validation work. Preserve the existing error mapping by parsing only after authorization succeeds, using the existing `ensure_memory_mount` and `from_tool_input` flow in this handler.Source: Coding guidelines
crates/ironclaw_memory_native/src/service.rs (2)
682-689: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNormalize disabled memory aliases before matching.
This is a boundary/config string; exact matching fails open for
Memory-Disabledor padded values and retrieves context when memory should be disabled.As per coding guidelines, “When comparing user-supplied strings … normalize to lowercase with
.to_ascii_lowercase()for case-insensitive comparison.”Suggested fix
- MEMORY_DISABLED_ALIASES.contains(&context_profile_id) + MEMORY_DISABLED_ALIASES + .iter() + .any(|alias| context_profile_id.trim().eq_ignore_ascii_case(alias))🤖 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_native/src/service.rs` around lines 682 - 689, The memory disable check in memory_context_disabled currently does an exact string match, so mixed-case or padded context profile IDs can bypass the disabled path. Normalize the input before comparing by trimming whitespace and converting context_profile_id to lowercase, then match against the existing MEMORY_DISABLED_ALIASES values using the normalized value so disabled memory aliases like Memory-Disabled are handled consistently.Source: Coding guidelines
105-117: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFilter and redact provider results before service DTOs leave the facade.
search/treestill serialize backend-returned paths without scope/protected-path filtering,searchcan return non-finite scores,retrieve_contextcan admit protected docs, andwritestill echoesBOOTSTRAP.md/HEARTBEAT.md. Add a single output guard forscope == context.scope(),score.is_finite(), and case-insensitive protected paths (AGENTS.md,HEARTBEAT.md,BOOTSTRAP.md,.system/), and map alias responses without internal filenames.As per coding guidelines, “Preserve tenant/user/agent/project/mission/thread scope…” and “Never expose to users … internal file names (
.system/,AGENTS.md,HEARTBEAT.md,BOOTSTRAP.md).”Suggested guard shape
+fn is_protected_memory_path(relative_path: &str) -> bool { + let normalized = relative_path.trim_matches('/').to_ascii_lowercase(); + matches!( + normalized.as_str(), + "agents.md" | "heartbeat.md" | "bootstrap.md" + ) || normalized.starts_with(".system/") +} + let results = self .backend .search(&context, search_request) .await .map_err(MemoryServiceError::operation_from)? .into_iter() + .filter(|result| { + result.path.scope() == context.scope() + && result.score.is_finite() + && !is_protected_memory_path(result.path.relative_path()) + }) .map(|result| MemoryServiceSearchResult {let mut paths = self .backend .list_documents(&context, &scope) .await .map_err(MemoryServiceError::operation_from)? .into_iter() + .filter(|path| { + path.scope() == context.scope() + && !is_protected_memory_path(path.relative_path()) + }) .map(|path| path.relative_path().to_string())- results.retain(|result| result.path.scope() == context.scope() && result.score.is_finite()); + results.retain(|result| { + result.path.scope() == context.scope() + && result.score.is_finite() + && !is_protected_memory_path(result.path.relative_path()) + });Also applies to: 135-153, 202-205, 245-255, 332-344
🤖 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_native/src/service.rs` around lines 105 - 117, Add a shared output-filtering guard in the MemoryService facade so backend results are sanitized before any DTOs leave the service. Update the search, tree, retrieve_context, and write paths to enforce scope == context.scope(), reject non-finite scores in search, and filter case-insensitive protected/internal paths like AGENTS.md, HEARTBEAT.md, BOOTSTRAP.md, and .system/ before mapping into MemoryServiceSearchResult or other public responses. Also ensure any alias handling in these methods does not leak internal filenames, and apply the same guard consistently across the affected MemoryService methods and DTO construction points.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_native/CLAUDE.md`:
- Line 8: Narrow the ownership statement in CLAUDE.md so it no longer claims
this crate owns the `/memory` virtual path grammar. Update the line to reflect
the native crate’s actual scope by referencing the filesystem-facing
parsing/adapter boundary and indexer hook boundaries, while leaving `/memory`
grammar and validation ownership aligned with the contract crate and the public
path surface in `path.rs`.
In `@crates/ironclaw_memory_native/src/events.rs`:
- Around line 23-24: The significant-event sink failure path in
record_memory_significant_event is logging the raw MemoryEventSinkError with
%error, which can leak backend or host details. Update the tracing::debug! call
to emit a stable, non-sensitive error code or classification instead of the raw
error value, and keep the log message generic while preserving enough context to
identify the failure location.
In `@crates/ironclaw_memory/src/context.rs`:
- Around line 37-43: `with_audit_context()` is accepting a `ResourceScope` that
can differ from the context’s own `self.scope`, which can make authorization and
audit attribution disagree. Update `Context::with_audit_context` to validate the
passed `resource_scope` against `self.scope` and reject mismatches, and stop
storing a separate `invocation_id` copy by deriving `invocation_id()` from the
same source of truth used by `audit_context.resource_scope`.
In `@crates/ironclaw_memory/src/hash.rs`:
- Around line 5-13: Add a small regression test in the hash module to lock the
wire format returned by content_bytes_sha256, since backend CAS logic compares
against that exact "sha256:<hex>" shape. Use a known input with a fixed expected
digest to assert the prefix and lowercase hex formatting, and add a parity check
that content_sha256 and content_bytes_sha256 produce the same result for
equivalent data. Reference the content_sha256 and content_bytes_sha256
entrypoints so future changes to casing or prefix are caught.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 253-280: The validation in MemoryContextProfileId::validate only
rejects empty strings, so whitespace-only IDs still pass through
MemoryContextProfileId::new and TryFrom<String>. Update the boundary check in
validate to treat trimmed-whitespace values as invalid, and keep the existing
constructors using that shared validation path so serde and direct construction
both reject blank-after-trim profile IDs consistently.
---
Outside diff comments:
In `@crates/ironclaw_memory_native/tests/memory_backend_contract.rs`:
- Around line 243-308: Add a mirrored regression test for append_file that
exercises the same adapter/backend prompt-safety handoff as
backend_does_not_re_reject_adapter_approved_warn_or_bypass. Use
MemoryBackendFilesystemAdapter, RepositoryMemoryBackend, and
PromptWriteSafetyPolicy to verify an adapter-approved append is not re-evaluated
by the backend when prompt_safety_already_enforced should be set. Make the
backend use an AlwaysRejectIfRecheckedPolicy and assert the append succeeds
through the caller path, covering the append_file retry/backend branch
separately from write_file.
In `@crates/ironclaw_memory/src/events.rs`:
- Around line 158-175: The search telemetry API currently uses two adjacent
bools in MemorySignificantEvent::search_performed, which makes the mode contract
easy to misuse; replace full_text/vector with a typed search mode value (small
enum or struct) and thread that through the call site in backend.rs around the
search event creation. Update both the event constructor and its caller so the
mode is explicit and impossible to swap accidentally, while keeping the
result_count and other fields unchanged.
---
Duplicate comments:
In `@crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs`:
- Around line 2079-2084: Add the same boundary restrictions to
ironclaw_memory_native wherever the corresponding ironclaw_memory rule is
currently blocked, since the native provider can otherwise bypass the intended
module boundary. Update the relevant BoundaryRule entries in
reborn_dependency_boundaries.rs so the denylist/forbidden targets mirror between
ironclaw_memory and ironclaw_memory_native, using the existing BoundaryRule
patterns around the affected crate_name checks. Ensure any guardrail coverage
also asserts the native provider cannot depend on host composition, dispatch, or
higher-authority subsystems when ironclaw_memory is forbidden.
In `@crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs`:
- Around line 33-38: The `/memory` write check in profile_set is happening after
untrusted input parsing, so move `ensure_memory_mount(request, /* write */
true)?` to run before
`MemoryServiceProfileSetRequest::from_tool_input(&request.input)` and keep the
request rejected with `FilesystemDenied` before any validation work. Preserve
the existing error mapping by parsing only after authorization succeeds, using
the existing `ensure_memory_mount` and `from_tool_input` flow in this handler.
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 90-98: The memory_context_disabled gate is doing an exact alias
match, so case or whitespace variants like Memory-Disabled can slip through and
still reach provider calls. Update memory_context_disabled to normalize the
incoming context_profile_id before checking MEMORY_DISABLED_ALIASES by trimming
whitespace and converting to lowercase (using to_ascii_lowercase), then compare
against the normalized aliases so the gate fails closed. Keep the fix localized
to memory_context_disabled and its alias list in memory_context.rs.
- Around line 23-26: The memory admission path in memory_context.rs is accepting
provider-controlled inputs without enough bounds: cap the forwarded
max_snippets, ensure invalid snippets still count toward the admission limit so
admitted.len() cannot be bypassed, and enforce a byte limit on snippet_ref
before it is accepted. Update the logic around the memory snippet admission flow
and the MEMORY_CONTEXT_REF_PREFIX handling to reject oversized refs and stop
untrusted providers from forcing excessive work or exceeding the content budget.
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 682-689: The memory disable check in memory_context_disabled
currently does an exact string match, so mixed-case or padded context profile
IDs can bypass the disabled path. Normalize the input before comparing by
trimming whitespace and converting context_profile_id to lowercase, then match
against the existing MEMORY_DISABLED_ALIASES values using the normalized value
so disabled memory aliases like Memory-Disabled are handled consistently.
- Around line 105-117: Add a shared output-filtering guard in the MemoryService
facade so backend results are sanitized before any DTOs leave the service.
Update the search, tree, retrieve_context, and write paths to enforce scope ==
context.scope(), reject non-finite scores in search, and filter case-insensitive
protected/internal paths like AGENTS.md, HEARTBEAT.md, BOOTSTRAP.md, and
.system/ before mapping into MemoryServiceSearchResult or other public
responses. Also ensure any alias handling in these methods does not leak
internal filenames, and apply the same guard consistently across the affected
MemoryService methods and DTO construction points.
In `@crates/ironclaw_memory_native/tests/memory_service_facade.rs`:
- Around line 97-268: Add caller-level regression tests for the public
NativeMemoryService APIs, not just retrieve_context: exercise search and tree
with mocked backend results that include cross-scope entries, protected-path
entries, and non-finite scores so the DTO filtering is verified through the
caller. Reuse the existing NativeMemoryService, MockSearchBackend,
search_result/search_result_with_agent, and invocation helpers to assert only
valid in-scope, finite results survive in search and tree output, matching the
filtering already covered in retrieve_context.
🪄 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: d1ba95a8-adbd-4d7c-b4e8-8835d8c0498a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (51)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_memory/AGENTS.mdcrates/ironclaw_memory/CLAUDE.mdcrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rscrates/ironclaw_product_adapters/tests/product_adapter_contract.rsdocs/reborn/contracts/memory.mddocs/reborn/contracts/storage-placement.md
BenKurrek
force-pushed
the
reborn/memory-placement-m2-lift
branch
from
June 24, 2026 14:33
2c5ed5f to
6022dcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
crates/ironclaw_memory_native/src/service.rs (3)
682-689: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNormalize
context_profile_idbefore disabled-alias matching.Exact
contains()matching fails open for inputs likeMemory-Disabledor whitespace-padded aliases, soretrieve_context()can still return snippets when memory should be disabled.Suggested fix
fn memory_context_disabled(context_profile_id: &str) -> bool { const MEMORY_DISABLED_ALIASES: &[&str] = &[ "memory_disabled", "memory-disabled", "disabled_context", "context_disabled", ]; - MEMORY_DISABLED_ALIASES.contains(&context_profile_id) + let normalized = context_profile_id.trim(); + MEMORY_DISABLED_ALIASES + .iter() + .any(|alias| normalized.eq_ignore_ascii_case(alias)) }As per coding guidelines, “When comparing user-supplied strings (file paths, media types, 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_memory_native/src/service.rs` around lines 682 - 689, The disabled-context check in memory_context_disabled is doing exact alias matching, so user-supplied values with casing differences or surrounding whitespace can slip through and keep retrieve_context() enabled. Normalize context_profile_id before comparing by trimming and converting it to lowercase, then match against the MEMORY_DISABLED_ALIASES list using the normalized value so aliases like Memory-Disabled and padded inputs are correctly treated as disabled.Source: Coding guidelines
105-117: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReapply facade-side result guards before serializing backend output.
retrieve_context()already drops cross-scope rows and non-finite scores at Line 337, butsearch()/tree()trust the backend entirely. SinceMemoryBackendis now a provider boundary, one bad backend/index can leak another tenant/user/project’s paths/snippets, andsearch()can still emitNaN/infscores.Suggested fix
let results = self .backend .search(&context, search_request) .await .map_err(MemoryServiceError::operation_from)? .into_iter() + .filter(|result| { + result.path.scope() == context.scope() && result.score.is_finite() + }) .map(|result| MemoryServiceSearchResult { is_hybrid_match: result.is_hybrid(), content: result.snippet, score: result.score, path: result.path.relative_path().to_string(), @@ let mut paths = self .backend .list_documents(&context, &scope) .await .map_err(MemoryServiceError::operation_from)? .into_iter() + .filter(|path| path.scope() == context.scope()) .map(|path| path.relative_path().to_string()) .collect::<Vec<_>>();As per coding guidelines, “Preserve tenant/user/agent/project/mission/thread scope on authority, state, memory, process, network, outbound, resource, and event records.”
Also applies to: 245-252
🤖 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_native/src/service.rs` around lines 105 - 117, The search and tree result mapping in the Memory service is missing the same facade-side guards already used by retrieve_context(), so backend output can leak cross-scope snippets/paths and non-finite scores. Update the result handling in MemoryService::search() and the corresponding tree path to filter backend rows by the active scope before serializing, and reject any entries with NaN/inf scores. Keep the guard logic centralized around the existing MemoryService and MemoryBackend boundary so untrusted backend data is sanitized before building MemoryServiceSearchResult.Source: Coding guidelines
111-115: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not surface protected system filenames through service DTOs.
These responses currently echo internal names like
BOOTSTRAP.md/HEARTBEAT.mdback to callers (search,tree, and alias writes). This facade is the host-facing tool surface, so those paths need alias mapping or suppression before serialization.As per coding guidelines, “Never expose to users: raw 5xx HTTP codes, Python tracebacks, absolute paths ... internal file names (
.system/,AGENTS.md,HEARTBEAT.md,BOOTSTRAP.md) ...”.Also applies to: 146-153, 202-205, 251-251
🤖 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_native/src/service.rs` around lines 111 - 115, The service DTOs are leaking protected internal filenames such as BOOTSTRAP.md and HEARTBEAT.md through search, tree, and alias write responses. Update the serialization/mapping in MemoryService (including the result mapping around MemoryServiceSearchResult and the tree/alias write paths) so internal paths are either suppressed or translated through the existing alias mapping before being returned to callers. Ensure any path exposed from relative_path() or similar helpers is filtered through the host-facing facade and never returns raw protected system filenames.Source: Coding guidelines
crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs (1)
33-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize
/memorybefore parsing profile input.Line 36 validates untrusted JSON before Line 38 checks the write grant, so unauthorized callers can observe profile-field validation instead of failing closed.
As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment...”
Fix ordering
- // Validate the profile fields first, then authorize the `/memory` write — - // matching the pre-lift ordering (`validated_fields` ran before - // `ensure_memory_mount`). - let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) - .map_err(map_memory_service_error)?; ensure_memory_mount(request, /* write */ true)?; + let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) + .map_err(map_memory_service_error)?;🤖 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/first_party_tools/profile_set.rs` around lines 33 - 38, The auth check in `profile_set` is happening after `MemoryServiceProfileSetRequest::from_tool_input`, which lets untrusted input be parsed before verifying the `/memory` write grant. Reorder `ensure_memory_mount(request, /* write */ true)?` to run before any profile-input parsing or validation, and keep `map_memory_service_error` handling only after authorization succeeds.Source: Coding guidelines
crates/ironclaw_host_runtime/src/memory_context.rs (1)
23-26: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCap provider-controlled
snippet_refbefore admission.Line 104 accepts arbitrarily large refs after only prefix/character checks; refs are not counted in the snippet or aggregate content budgets.
As per coding guidelines, “Keep runtime crates untrusted; host-runtime must mediate secrets/network/redaction/accounting.”
Add a ref byte cap
const MAX_MEMORY_CONTEXT_SNIPPET_BYTES: usize = 512; const MAX_MEMORY_CONTEXT_TOTAL_BYTES: usize = 4 * 1024; +const MAX_MEMORY_CONTEXT_REF_BYTES: usize = 256; const MEMORY_CONTEXT_REF_PREFIX: &str = "memory-snippet:"; const MEMORY_CONTEXT_UNTRUSTED_PREFIX: &str = "Untrusted memory content:"; @@ - if !snippet.snippet_ref.starts_with(MEMORY_CONTEXT_REF_PREFIX) + if snippet.snippet_ref.len() > MAX_MEMORY_CONTEXT_REF_BYTES + || !snippet.snippet_ref.starts_with(MEMORY_CONTEXT_REF_PREFIX) || snippet.snippet_ref.chars().any(|character| {Also applies to: 104-105
🤖 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_context.rs` around lines 23 - 26, The memory context admission path in memory_context.rs allows provider-controlled snippet_ref values to bypass budgeting after only prefix/character validation. Add a hard byte-length cap for refs in the admission logic that handles memory snippet refs (the code around the snippet_ref check and MEMORY_CONTEXT_REF_PREFIX), and reject any ref exceeding it before it is stored or counted. Keep the existing content/aggregate accounting intact while ensuring oversized refs cannot be admitted.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_native/AGENTS.md`:
- Around line 14-20: Narrow the ownership statement in AGENTS.md so it only
covers the native adapter boundary and does not claim ownership of the `/memory`
grammar or value types that belong to the contract crate. Update the scoped
bullets around MemoryDocumentRepository, MemoryBackend,
MemoryDocumentFilesystem, and related adapter pieces, and remove or reword the
references to MemoryDocumentPath, MemoryDocumentScope, and the path grammar so
the docs align with the module boundary.
In `@crates/ironclaw_memory_native/src/metadata.rs`:
- Around line 80-93: find_nearest_config currently re-resolves the same .config
file for path values like dir/.config, causing the function to return the
current document instead of the nearest parent/root config. Update the loop in
find_nearest_config so it skips the current CONFIG_FILE_NAME entry before
checking ancestors, and only searches parent directories plus the root fallback.
Use the existing find_nearest_config function, the CONFIG_FILE_NAME constant,
and the current/current.rfind('/') traversal logic to locate and adjust the
ancestor resolution.
In `@docs/reborn/contracts/memory.md`:
- Around line 14-20: Clarify the crate ownership in the memory contract doc so
the responsibility list is not read as belonging to ironclaw_memory; update the
section around the crate split summary and the bullets to explicitly separate
provider-neutral contract responsibilities in ironclaw_memory from concrete
implementation work in ironclaw_memory_native. Reword the intro and bullet
framing, using the MemoryService trait and the native implementation
responsibilities as the anchors, so readers understand that contract definitions
stay in ironclaw_memory while
repository/chunking/search/prompt-context/bootstrap work belongs to
ironclaw_memory_native.
---
Duplicate comments:
In `@crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs`:
- Around line 33-38: The auth check in `profile_set` is happening after
`MemoryServiceProfileSetRequest::from_tool_input`, which lets untrusted input be
parsed before verifying the `/memory` write grant. Reorder
`ensure_memory_mount(request, /* write */ true)?` to run before any
profile-input parsing or validation, and keep `map_memory_service_error`
handling only after authorization succeeds.
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 23-26: The memory context admission path in memory_context.rs
allows provider-controlled snippet_ref values to bypass budgeting after only
prefix/character validation. Add a hard byte-length cap for refs in the
admission logic that handles memory snippet refs (the code around the
snippet_ref check and MEMORY_CONTEXT_REF_PREFIX), and reject any ref exceeding
it before it is stored or counted. Keep the existing content/aggregate
accounting intact while ensuring oversized refs cannot be admitted.
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 682-689: The disabled-context check in memory_context_disabled is
doing exact alias matching, so user-supplied values with casing differences or
surrounding whitespace can slip through and keep retrieve_context() enabled.
Normalize context_profile_id before comparing by trimming and converting it to
lowercase, then match against the MEMORY_DISABLED_ALIASES list using the
normalized value so aliases like Memory-Disabled and padded inputs are correctly
treated as disabled.
- Around line 105-117: The search and tree result mapping in the Memory service
is missing the same facade-side guards already used by retrieve_context(), so
backend output can leak cross-scope snippets/paths and non-finite scores. Update
the result handling in MemoryService::search() and the corresponding tree path
to filter backend rows by the active scope before serializing, and reject any
entries with NaN/inf scores. Keep the guard logic centralized around the
existing MemoryService and MemoryBackend boundary so untrusted backend data is
sanitized before building MemoryServiceSearchResult.
- Around line 111-115: The service DTOs are leaking protected internal filenames
such as BOOTSTRAP.md and HEARTBEAT.md through search, tree, and alias write
responses. Update the serialization/mapping in MemoryService (including the
result mapping around MemoryServiceSearchResult and the tree/alias write paths)
so internal paths are either suppressed or translated through the existing alias
mapping before being returned to callers. Ensure any path exposed from
relative_path() or similar helpers is filtered through the host-facing facade
and never returns raw protected system filenames.
🪄 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: f68fa73a-b05e-4358-9635-37dfc2b0ef24
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (51)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_memory/AGENTS.mdcrates/ironclaw_memory/CLAUDE.mdcrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rscrates/ironclaw_product_adapters/tests/product_adapter_contract.rsdocs/reborn/contracts/memory.mddocs/reborn/contracts/storage-placement.md
serrrfirat
reviewed
Jun 24, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Collaborator
There was a problem hiding this comment.
Multi-agent review plus thermo-nuclear maintainability pass for #5163.
Summary:
- Security: 1 Medium finding
- Tests: 2 Medium findings, 1 Low finding
- Conventions/maintainability: 2 Medium findings
- Bugs and performance/concurrency reviewers returned no findings
Verification I ran locally on the PR head:
- cargo test -p ironclaw_memory -p ironclaw_memory_native
- cargo test -p ironclaw_architecture --test reborn_dependency_boundaries
- cargo test -p ironclaw_host_runtime profile_set
- cargo test -p ironclaw_host_runtime --test memory_prompt_context
Event: COMMENT because there are no Critical/High findings after aggregation.
henrypark133
requested changes
Jun 24, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Lift the Reborn memory layer into a provider-neutral userland extension crate and native filesystem provider while preserving existing memory-tool behavior exactly.
Stats: 2 findings (from 6 raw, 2 after dedup/verification) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1
Bugs
- High Empty patch replacement text is now accepted (
crates/ironclaw_memory/src/service.rs:94-97, confidence 96) — anchor:crates/ironclaw_memory/src/service.rs:94
new_stringis stored verbatim, so a patch request with"new_string": ""now reaches the native provider and is applied as an empty replacement. The pre-lift host path usedrequired_str(input, "new_string"), which rejected empty replacements, so this violates the PR's behavior-preserving contract and lets a previously invalidmemory_writerequest delete matched text.
Maintainability
- Medium Centralize the disabled-context aliases (
crates/ironclaw_memory_native/src/service.rs:682-689, confidence 88) — anchor:crates/ironclaw_host_runtime/src/memory_context.rs:90(no diff position — body only)
The disabled memory-context profile aliases are duplicated in the host-runtime gate and the native provider defense-in-depth check. Those two checks are meant to protect the same behavior-preserving invariant, but adding or removing an alias in only one place would silently desynchronize whether snippet retrieval is blocked before and inside the provider.
BenKurrek
force-pushed
the
reborn/memory-placement-m2-lift
branch
from
June 24, 2026 15:44
6022dcd to
236a30f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/ironclaw_memory/src/service.rs (1)
128-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale write-status doc above
MemoryProfileSetStatus.Line 128 still says “Outcome class of a memory write operation,” but this enum is the
profile_setstatus. That leaves the public contract self-contradictory for downstream readers.🤖 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 128 - 131, The documentation above MemoryProfileSetStatus is stale and contradicts the enum’s actual purpose. Update the doc comment so it clearly describes the status of the profile_set operation, and remove the generic “memory write operation” wording to keep the public contract aligned with the enum’s behavior and the existing note about the native provider only reporting success.crates/ironclaw_host_runtime/src/memory_context.rs (1)
23-26: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCap provider-controlled
snippet_refbefore admission.
snippet_refis not included in the 512-byte snippet or 4 KiB aggregate content budgets, but Line 145 later admits it intoLoopContextSnippet. Add a small byte cap before the prefix/character scan so a provider cannot return a huge ref that bypasses host accounting.As per coding guidelines, “Keep runtime crates untrusted; host-runtime must mediate secrets/network/redaction/accounting.”
Proposed fix
const MAX_MEMORY_CONTEXT_SNIPPET_BYTES: usize = 512; const MAX_MEMORY_CONTEXT_TOTAL_BYTES: usize = 4 * 1024; +const MAX_MEMORY_CONTEXT_REF_BYTES: usize = 256; const MEMORY_CONTEXT_REF_PREFIX: &str = "memory-snippet:"; @@ - if !snippet.snippet_ref.starts_with(MEMORY_CONTEXT_REF_PREFIX) + if snippet.snippet_ref.len() > MAX_MEMORY_CONTEXT_REF_BYTES + || !snippet.snippet_ref.starts_with(MEMORY_CONTEXT_REF_PREFIX) || snippet.snippet_ref.chars().any(|character| {Also applies to: 90-105
🤖 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_context.rs` around lines 23 - 26, Cap provider-controlled snippet_ref before it is admitted into LoopContextSnippet: the current snippet/content limits in memory_context.rs only cover the 512-byte snippet and 4 KiB aggregate text, so a large snippet_ref can bypass host accounting. Add a small byte-length guard in the admission path around the prefix/character scan in the snippet handling logic (including the code touched in the 90-105 and 145 areas) and reject or truncate oversized refs before they are stored.Source: Coding guidelines
crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs (1)
33-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize
/memorybefore parsing profile input.Line 36 validates untrusted JSON before Line 38 checks the
/memorywrite grant, so unauthorized callers can still get profile-field validation feedback instead of failing closed withFilesystemDenied. Moveensure_memory_mount(request, true)beforeMemoryServiceProfileSetRequest::from_tool_input(...).As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment...”
Proposed fix
- // Validate the profile fields first, then authorize the `/memory` write — - // matching the pre-lift ordering (`validated_fields` ran before - // `ensure_memory_mount`). - let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) - .map_err(map_memory_service_error)?; ensure_memory_mount(request, /* write */ true)?; + let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) + .map_err(map_memory_service_error)?;🤖 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/first_party_tools/profile_set.rs` around lines 33 - 38, In profile_set.rs, the authorization check for `/memory` must happen before any untrusted profile JSON is parsed. Update the profile set flow in the `profile_request` setup so `ensure_memory_mount(request, true)` is called before `MemoryServiceProfileSetRequest::from_tool_input(...)`, keeping `map_memory_service_error` on the parsing step afterward. This ensures `MemoryServiceProfileSetRequest` cannot return validation feedback to callers who have not already passed the `/memory` write grant check.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_native/src/lib.rs`:
- Around line 1-5: The crate-level docs in lib.rs are too broad and incorrectly
claim ownership of memory path grammar; narrow the wording so this native
adapter crate only describes repository/filesystem seams and adapter
responsibilities. Update the top-level documentation comment to remove any
mention of owning path grammar, and align the description with the native
adapter boundary exposed by the crate entrypoint in lib.rs.
---
Duplicate comments:
In `@crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs`:
- Around line 33-38: In profile_set.rs, the authorization check for `/memory`
must happen before any untrusted profile JSON is parsed. Update the profile set
flow in the `profile_request` setup so `ensure_memory_mount(request, true)` is
called before `MemoryServiceProfileSetRequest::from_tool_input(...)`, keeping
`map_memory_service_error` on the parsing step afterward. This ensures
`MemoryServiceProfileSetRequest` cannot return validation feedback to callers
who have not already passed the `/memory` write grant check.
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 23-26: Cap provider-controlled snippet_ref before it is admitted
into LoopContextSnippet: the current snippet/content limits in memory_context.rs
only cover the 512-byte snippet and 4 KiB aggregate text, so a large snippet_ref
can bypass host accounting. Add a small byte-length guard in the admission path
around the prefix/character scan in the snippet handling logic (including the
code touched in the 90-105 and 145 areas) and reject or truncate oversized refs
before they are stored.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 128-131: The documentation above MemoryProfileSetStatus is stale
and contradicts the enum’s actual purpose. Update the doc comment so it clearly
describes the status of the profile_set operation, and remove the generic
“memory write operation” wording to keep the public contract aligned with the
enum’s behavior and the existing note about the native provider only reporting
success.
🪄 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: 48264cc5-ab44-4ca9-aad6-bb303a27d220
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (52)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_memory/AGENTS.mdcrates/ironclaw_memory/CLAUDE.mdcrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rscrates/ironclaw_product_adapters/tests/product_adapter_contract.rsdocs/reborn/contracts/memory.mddocs/reborn/contracts/storage-placement.md
BenKurrek
enabled auto-merge
June 24, 2026 16:28
BenKurrek
force-pushed
the
reborn/memory-placement-m2-lift
branch
from
June 24, 2026 17:50
bd8906d to
f3fac01
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (8)
crates/ironclaw_memory/src/service.rs (4)
128-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove the write-status docs back above
MemoryWriteStatus.The leading sentence here describes the write enum, but it now sits on
MemoryProfileSetStatus. That makes the public contract misleading for downstream implementers.Proposed fix
-/// Outcome class of a memory write operation. -/// /// Status of a `profile_set` operation. The native provider only ever reports /// success (`ok`); a failed write surfaces as an error, not a status. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum MemoryProfileSetStatus { Ok, } +/// Outcome class of a memory write operation. +/// /// Serializes to exactly `"cleared"` / `"written"` / `"patched"` via serde /// snake_case, preserving the historical wire format that previously lived in /// a `String` status field.🤖 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 128 - 131, The write-status documentation is attached to the wrong type, so move the leading “Outcome class of a memory write operation” docs back to MemoryWriteStatus and keep the MemoryProfileSetStatus docs focused on profile_set behavior. Update the doc comments in service.rs around MemoryWriteStatus and MemoryProfileSetStatus so each public contract matches the correct enum, without changing the enum definitions themselves.
35-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail loud on malformed numeric options.
optional_u64()drops present-but-invalidlimit/depthintoNone, so both parsers silently fall back to defaults. That hides boundary errors instead of rejecting them. As per path instructions, “Fail loud: flag silent-failure patterns.”Proposed fix
- let limit = optional_u64(input, "limit").unwrap_or(5).clamp(1, 20) as usize; + let limit = optional_u64(input, "limit")?.unwrap_or(5).clamp(1, 20) as usize; @@ - let depth = optional_u64(input, "depth").unwrap_or(1).clamp(1, 10) as usize; + let depth = optional_u64(input, "depth")?.unwrap_or(1).clamp(1, 10) as usize; @@ -fn optional_u64(input: &Value, key: &'static str) -> Option<u64> { - input.get(key).and_then(Value::as_u64) +fn optional_u64(input: &Value, key: &'static str) -> Result<Option<u64>, MemoryServiceError> { + match input.get(key) { + None | Some(Value::Null) => Ok(None), + Some(Value::Number(number)) => number + .as_u64() + .map(Some) + .ok_or_else(MemoryServiceError::input), + Some(_) => Err(MemoryServiceError::input()), + } }Also applies to: 208-208, 495-496
🤖 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 35 - 36, The numeric option parsing currently hides malformed `limit` and `depth` values by using `optional_u64()` and falling back to defaults, so update the parsers in `service.rs` to fail loudly instead of treating invalid present values as missing. Adjust the relevant request/command parsing logic that constructs `Self { query, limit }` and the other `depth`-related parser sites so that invalid numeric input returns an error, while only truly absent fields use defaults.Source: Path instructions
203-208: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject present non-string
pathinstead of listing the scope root.
path: 123currently becomes"", so a malformed subtree request is broadened into a root tree listing. Missing/null can still mean root, but present wrong-typed input should fail at this boundary. As per path instructions, “Fail loud: flag silent-failure patterns.”Proposed fix
- let path = input - .get("path") - .and_then(Value::as_str) - .unwrap_or("") - .to_string(); + let path = match input.get("path") { + None | Some(Value::Null) => String::new(), + Some(Value::String(path)) => path.clone(), + Some(_) => return Err(MemoryServiceError::input()), + };🤖 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 203 - 208, The subtree request handling in service.rs currently treats a present non-string path as an empty string, which silently turns malformed input into a root listing. Update the path parsing in the subtree flow so that the relevant request handler rejects a present path value unless it is a string, while still allowing missing or null path to default to root. Use the existing input parsing logic around input.get("path"), Value::as_str, and optional_u64 in the same request boundary to return a validation error instead of falling back to "" for wrong-typed input.Source: Path instructions
269-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim whitespace in
MemoryContextProfileIdvalidation.
" "currently passesvalidate()and crosses the host→provider boundary as a “validated” id. Boundary constructors should reject blank-after-trim values. As per coding guidelines, “Keep value-type constructors validating at the boundary; do not add unchecked public constructors.”Proposed fix
fn validate(value: &str) -> Result<(), MemoryServiceError> { - if value.is_empty() { + if value.trim().is_empty() { return Err(MemoryServiceError::input()); } Ok(()) }🤖 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 269 - 275, The MemoryContextProfileId::validate boundary check only rejects empty strings, so whitespace-only values can still be treated as valid ids; update this validator to trim the input before checking so blank-after-trim values are rejected with MemoryServiceError::input(). Keep the fix localized to MemoryContextProfileId::validate and preserve the existing value-type boundary validation behavior without introducing any unchecked constructor.Source: Coding guidelines
crates/ironclaw_memory_native/AGENTS.md (1)
14-19: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNarrow the ownership claim to native seams.
This still assigns
MemoryDocumentPath/MemoryDocumentScopeand the/memorygrammar toironclaw_memory_native, but the split in this PR moves those public contract types intoironclaw_memory. Keep this file scoped to repository/backend/filesystem/indexer ownership so the crate-local guardrails do not fight each other.🤖 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_native/AGENTS.md` around lines 14 - 19, Narrow the ownership statement in AGENTS.md so ironclaw_memory_native only claims the native seams it actually owns. Remove the /memory path grammar and MemoryDocumentPath/MemoryDocumentScope references from this crate’s guardrails, and keep the scope centered on MemoryDocumentRepository, MemoryBackend, filesystem adapter, chunking/indexer, embedding, search, events, and safety types that remain local to ironclaw_memory_native.crates/ironclaw_memory_native/CLAUDE.md (1)
8-8: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDrop the
/memorygrammar ownership claim here.The surrounding guardrail text says the scope/path/context value types live in
ironclaw_memory, but this line still claims the native crate owns the/memoryvirtual path grammar. Reword it to the filesystem-facing parser/adapter boundary so the two crate-local specs stay aligned.🤖 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_native/CLAUDE.md` at line 8, Update the scope statement in CLAUDE.md for ironclaw_memory_native so it no longer claims ownership of the /memory virtual path grammar; rephrase that responsibility to the filesystem-facing parser/adapter boundary instead. Keep the rest of the list aligned with the crate’s actual seams, and use the existing terms like memory document repository seams, memory-document filesystem adapters, and indexer hook boundaries to preserve consistency with the ironclaw_memory spec.crates/ironclaw_memory_native/src/safety.rs (1)
405-412: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStop logging raw prompt-safety sink errors.
Line 408 leaks a boundary error back into structured logs.
MemoryEventSinkErroris free-form, so%errorcan surface backend paths, secrets, or unredacted payloads from the sink implementation. Log a stable sanitized code instead.As per coding guidelines, "Do not include secrets, raw host paths, backend error details, or unredacted user content in errors, events, snapshots, logs, or documentation."
Suggested fix
- if let Err(error) = event_sink.record_prompt_write_safety_event(event).await { + if event_sink.record_prompt_write_safety_event(event).await.is_err() { tracing::debug!( target: "ironclaw::memory::prompt_write_safety", - error = %error, + error = "prompt_write_safety_event_sink_failed", operation = %check.operation, source = %check.source, "failed to record prompt write safety event" );🤖 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_native/src/safety.rs` around lines 405 - 412, The prompt-write safety logging in MemoryEventSinkError handling is leaking raw sink error details into structured logs. Update the error path in safety.rs around record_prompt_write_safety_event to stop using %error in tracing::debug and instead log a stable sanitized failure code or enum-derived label, while keeping operation and source context from check.operation and check.source. Use the existing record_prompt_write_safety_event and tracing::debug callsite to locate the change and ensure no backend-specific or unredacted details are emitted.Source: Coding guidelines
crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs (1)
33-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize
/memorybefore parsing profile input.Line 36 validates untrusted fields before Line 38 checks the write grant, so unauthorized callers can still learn profile-field validation details instead of failing closed.
Proposed fix
- // Validate the profile fields first, then authorize the `/memory` write — - // matching the pre-lift ordering (`validated_fields` ran before - // `ensure_memory_mount`). - let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) - .map_err(map_memory_service_error)?; ensure_memory_mount(request, /* write */ true)?; + let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) + .map_err(map_memory_service_error)?;As per coding guidelines, “Fail closed for auth, approvals, trust, filesystem containment...”. As per path instructions, review against the repo invariant “Fail loud”.
🤖 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/first_party_tools/profile_set.rs` around lines 33 - 38, Authorize the `/memory` write before parsing any profile input in `MemoryServiceProfileSetRequest::from_tool_input`; move `ensure_memory_mount(request, /* write */ true)?` ahead of the request parsing in `profile_set.rs` so unauthorized callers fail closed before seeing validation behavior. Keep the existing `map_memory_service_error` handling after authorization, and preserve the current profile-set flow around `profile_request` and `ensure_memory_mount`.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.
Duplicate comments:
In `@crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs`:
- Around line 33-38: Authorize the `/memory` write before parsing any profile
input in `MemoryServiceProfileSetRequest::from_tool_input`; move
`ensure_memory_mount(request, /* write */ true)?` ahead of the request parsing
in `profile_set.rs` so unauthorized callers fail closed before seeing validation
behavior. Keep the existing `map_memory_service_error` handling after
authorization, and preserve the current profile-set flow around
`profile_request` and `ensure_memory_mount`.
In `@crates/ironclaw_memory_native/AGENTS.md`:
- Around line 14-19: Narrow the ownership statement in AGENTS.md so
ironclaw_memory_native only claims the native seams it actually owns. Remove the
/memory path grammar and MemoryDocumentPath/MemoryDocumentScope references from
this crate’s guardrails, and keep the scope centered on
MemoryDocumentRepository, MemoryBackend, filesystem adapter, chunking/indexer,
embedding, search, events, and safety types that remain local to
ironclaw_memory_native.
In `@crates/ironclaw_memory_native/CLAUDE.md`:
- Line 8: Update the scope statement in CLAUDE.md for ironclaw_memory_native so
it no longer claims ownership of the /memory virtual path grammar; rephrase that
responsibility to the filesystem-facing parser/adapter boundary instead. Keep
the rest of the list aligned with the crate’s actual seams, and use the existing
terms like memory document repository seams, memory-document filesystem
adapters, and indexer hook boundaries to preserve consistency with the
ironclaw_memory spec.
In `@crates/ironclaw_memory_native/src/safety.rs`:
- Around line 405-412: The prompt-write safety logging in MemoryEventSinkError
handling is leaking raw sink error details into structured logs. Update the
error path in safety.rs around record_prompt_write_safety_event to stop using
%error in tracing::debug and instead log a stable sanitized failure code or
enum-derived label, while keeping operation and source context from
check.operation and check.source. Use the existing
record_prompt_write_safety_event and tracing::debug callsite to locate the
change and ensure no backend-specific or unredacted details are emitted.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 128-131: The write-status documentation is attached to the wrong
type, so move the leading “Outcome class of a memory write operation” docs back
to MemoryWriteStatus and keep the MemoryProfileSetStatus docs focused on
profile_set behavior. Update the doc comments in service.rs around
MemoryWriteStatus and MemoryProfileSetStatus so each public contract matches the
correct enum, without changing the enum definitions themselves.
- Around line 35-36: The numeric option parsing currently hides malformed
`limit` and `depth` values by using `optional_u64()` and falling back to
defaults, so update the parsers in `service.rs` to fail loudly instead of
treating invalid present values as missing. Adjust the relevant request/command
parsing logic that constructs `Self { query, limit }` and the other
`depth`-related parser sites so that invalid numeric input returns an error,
while only truly absent fields use defaults.
- Around line 203-208: The subtree request handling in service.rs currently
treats a present non-string path as an empty string, which silently turns
malformed input into a root listing. Update the path parsing in the subtree flow
so that the relevant request handler rejects a present path value unless it is a
string, while still allowing missing or null path to default to root. Use the
existing input parsing logic around input.get("path"), Value::as_str, and
optional_u64 in the same request boundary to return a validation error instead
of falling back to "" for wrong-typed input.
- Around line 269-275: The MemoryContextProfileId::validate boundary check only
rejects empty strings, so whitespace-only values can still be treated as valid
ids; update this validator to trim the input before checking so blank-after-trim
values are rejected with MemoryServiceError::input(). Keep the fix localized to
MemoryContextProfileId::validate and preserve the existing value-type boundary
validation behavior without introducing any unchecked constructor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71e9bc22-5796-4cfe-b535-4cc5c5dd5113
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (52)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_memory/AGENTS.mdcrates/ironclaw_memory/CLAUDE.mdcrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rscrates/ironclaw_product_adapters/tests/product_adapter_contract.rsdocs/reborn/contracts/memory.mddocs/reborn/contracts/storage-placement.md
💤 Files with no reviewable changes (2)
- docs/reborn/contracts/storage-placement.md
- docs/reborn/contracts/memory.md
Lift the Reborn memory layer out of the kernel into a provider-neutral contract crate (ironclaw_memory) plus a native filesystem provider crate (ironclaw_memory_native), routing the first-party memory tools and prompt-context retrieval through an Arc<dyn MemoryService> facade. Strictly behavior-preserving: a behavior-equivalence audit against origin/main confirms the memory tools' observable behavior — input parsing, response JSON, error kinds, stored-document semantics, and prompt-context retrieval — is unchanged. The native provider keeps the existing filesystem storage format. Out of scope (follow-ups): registering the native provider as a distinct extension manifest and wiring provider selection; the prompt_doc_ref manifest rule, memory-profile binding resolver, and host-port scaffolding were ripped out, and development-time "hardening" deviations from origin behavior were reverted to exact origin behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BenKurrek
force-pushed
the
reborn/memory-placement-m2-lift
branch
from
June 24, 2026 18:19
f3fac01 to
09f7731
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reborn/contracts/memory.md (1)
5-20: 📐 Maintainability & Code Quality | 🟡 MinorUpdate the memory split docs repo-wide
docs/reborn/contracts/memory.mdhas the split note, but the required.md/CLAUDE.mdsweep still leaves stale ownership wording indocs/reborn/contracts/storage-placement.md,docs/reborn/contracts/filesystem.md,docs/reborn/contracts/_contract-freeze-index.md,crates/AGENTS.md, andcrates/README.md. Those still describeironclaw_memoryas owning the concrete adapter/search/indexing path, which conflicts with the newironclaw_memory_nativeboundary. Keep the contract and crate docs aligned with the repo doc-hygiene invariant inCLAUDE.md/AGENTS.md.🤖 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 `@docs/reborn/contracts/memory.md` around lines 5 - 20, Update the repo-wide memory ownership docs so they reflect the new provider-neutral split: `ironclaw_memory` should be described only as the contract surface, while `ironclaw_memory_native` owns the concrete adapter/search/indexing/bootstrap responsibilities. Sweep the stale wording in `storage-placement.md`, `filesystem.md`, `_contract-freeze-index.md`, `crates/AGENTS.md`, and `crates/README.md`, and keep the language consistent with the split note already introduced in `memory.md`. Refer to the existing `MemoryService`/`ironclaw_memory_native` terminology so the docs stay aligned with the repo-wide doc-hygiene rules.Source: Coding guidelines
♻️ Duplicate comments (10)
docs/reborn/contracts/memory.md (1)
12-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe responsibility list still reads like the pre-split crate layout.
Line 12 scopes the section to
ironclaw_memory, but Lines 25-31 still list repository/chunking/prompt-context/bootstrap work that Lines 14-20 just moved intoironclaw_memory_native. Split these bullets by contract vs provider ownership, or rename the section as subsystem-wide responsibilities.Based on learnings,
ironclaw_memoryis the provider-neutral contract and concrete providers belong inironclaw_memory_native.🤖 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 `@docs/reborn/contracts/memory.md` around lines 12 - 32, The responsibilities list in the `ironclaw_memory` contract section still mixes provider-neutral contract duties with concrete implementation work. Update the bullets around the `MemoryService` split so `ironclaw_memory` only describes shared contracts/value types and provider-agnostic semantics, while repository seams, chunking/indexing/embeddings/search, prompt-context assembly, seeding/bootstrap/profile sync, and similar implementation-specific responsibilities are attributed to `ironclaw_memory_native`. Use the existing `ironclaw_memory` and `ironclaw_memory_native` split wording to keep the section aligned with the new crate ownership.Source: Learnings
crates/ironclaw_memory_native/src/service.rs (3)
105-116: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRe-apply scope validation before returning
search/treeDTOs.
retrieve_contextalready re-checksresult.path.scope() == context.scope(), butsearchandtreestill trust backend output. BecauseNativeMemoryService::newaccepts anyMemoryBackend, one buggy or alternate backend can leak another tenant/user/project’s snippets or filenames straight through the public facade.Minimal hardening
let results = self .backend .search(&context, search_request) .await .map_err(MemoryServiceError::operation_from)? .into_iter() + .filter(|result| result.path.scope() == context.scope()) .map(|result| MemoryServiceSearchResult { is_hybrid_match: result.is_hybrid(), content: result.snippet, score: result.score, path: result.path.relative_path().to_string(), @@ let mut paths = self .backend .list_documents(&context, &scope) .await .map_err(MemoryServiceError::operation_from)? .into_iter() + .filter(|path| path.scope() == context.scope()) .map(|path| path.relative_path().to_string()) .collect::<Vec<_>>();As per coding guidelines, “Preserve tenant/user/agent/project/mission/thread scope on authority, state, memory, process, network, outbound, resource, and event records.”
Also applies to: 250-257
🤖 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_native/src/service.rs` around lines 105 - 116, The search/tree DTO mapping in NativeMemoryService is trusting backend output and can leak data across scopes. Re-apply the same scope check used in retrieve_context before constructing MemoryServiceSearchResult and the tree DTOs, filtering out any results whose result.path.scope() does not match the current context.scope(). Keep the validation in the search and tree paths that call backend.search/backend.tree so all public returns preserve tenant/user/project scope.Source: Coding guidelines
111-116: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStop exposing protected internal filenames in public memory responses.
These DTO mappings still surface native names like
BOOTSTRAP.md/HEARTBEAT.mddirectly, and the bootstrap clear message at Line 152 echoesBOOTSTRAP.mdverbatim. This facade is serialized back to users, so protected files need alias mapping or suppression before the response is built.As per coding guidelines, “Never expose to users: raw 5xx HTTP codes, Python tracebacks, absolute paths (e.g.,
/workspace/...,/home/...), internal file names (.system/,AGENTS.md,HEARTBEAT.md,BOOTSTRAP.md), or wire-format prefixes...”Also applies to: 146-153, 207-214, 256-257
🤖 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_native/src/service.rs` around lines 111 - 116, The memory response DTOs and bootstrap-clear messaging are leaking protected internal filenames such as BOOTSTRAP.md and HEARTBEAT.md through public outputs. Update the mappings in MemoryService::search and the related bootstrap-clear flow to alias, redact, or suppress internal filenames before serializing MemoryServiceSearchResult or user-facing messages, using the existing service-layer transformation points rather than exposing path.relative_path() or raw file names directly.Source: Coding guidelines
111-116: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrop non-finite scores in
searchtoo.
retrieve_contextalready filtersNaN/inf, butsearchforwards raw scores intoMemoryServiceSearchResult. These responses are serialized back to tool JSON, so a single non-finite backend score can turn the whole search call into a failed response.Minimal hardening
let results = self .backend .search(&context, search_request) .await .map_err(MemoryServiceError::operation_from)? .into_iter() + .filter(|result| result.score.is_finite()) .map(|result| MemoryServiceSearchResult {🤖 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_native/src/service.rs` around lines 111 - 116, The `search` mapping in `MemoryServiceSearchResult` is forwarding raw backend scores without filtering, so non-finite values can break JSON responses. Update the `search` flow alongside `retrieve_context` to drop or skip results whose `result.score` is not finite before constructing `MemoryServiceSearchResult`, using the existing `search`/`MemoryServiceSearchResult` mapping as the place to apply the hardening.crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs (1)
33-38: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize
/memorybefore parsing profile input.Line 36 validates untrusted JSON before Line 38 checks the write grant, so unauthorized callers can receive profile-field validation feedback instead of failing closed with
FilesystemDenied.Proposed fix
- // Validate the profile fields first, then authorize the `/memory` write — - // matching the pre-lift ordering (`validated_fields` ran before - // `ensure_memory_mount`). - let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) - .map_err(map_memory_service_error)?; ensure_memory_mount(request, /* write */ true)?; + let profile_request = MemoryServiceProfileSetRequest::from_tool_input(&request.input) + .map_err(map_memory_service_error)?;As per path instructions, “Fail closed for auth, approvals, trust, filesystem containment...” and “Trusted-ingress seal” apply at this host-runtime boundary.
🤖 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/first_party_tools/profile_set.rs` around lines 33 - 38, Authorize the `/memory` write before parsing any profile input in `MemoryServiceProfileSetRequest` handling, so unauthorized callers fail closed with `FilesystemDenied` instead of getting validation errors. Move the `ensure_memory_mount(request, /* write */ true)?` check ahead of `MemoryServiceProfileSetRequest::from_tool_input(&request.input)` in `profile_set.rs`, keeping the existing error mapping and request flow intact.Sources: Coding guidelines, Path instructions
crates/ironclaw_host_runtime/src/memory_context.rs (1)
23-26: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap provider-controlled
snippet_refbefore admission.Line 94 only checks prefix/characters. A very large ref can pass outside both context byte budgets and enter
LoopContextSnippet.Proposed fix
const MAX_MEMORY_CONTEXT_SNIPPET_BYTES: usize = 512; const MAX_MEMORY_CONTEXT_TOTAL_BYTES: usize = 4 * 1024; +const MAX_MEMORY_CONTEXT_REF_BYTES: usize = 256; const MEMORY_CONTEXT_REF_PREFIX: &str = "memory-snippet:"; const MEMORY_CONTEXT_UNTRUSTED_PREFIX: &str = "Untrusted memory content:"; @@ - if !snippet.snippet_ref.starts_with(MEMORY_CONTEXT_REF_PREFIX) + if snippet.snippet_ref.len() > MAX_MEMORY_CONTEXT_REF_BYTES + || !snippet.snippet_ref.starts_with(MEMORY_CONTEXT_REF_PREFIX) || snippet.snippet_ref.chars().any(|character| {As per coding guidelines, “Keep runtime crates untrusted; host-runtime must mediate secrets/network/redaction/accounting.”
Also applies to: 90-105
🤖 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_context.rs` around lines 23 - 26, The snippet admission path in memory_context.rs only validates the provider-controlled snippet_ref prefix/characters, so a large ref can still bypass the byte budgets and reach LoopContextSnippet. Update the validation in the snippet handling flow around the snippet_ref check and LoopContextSnippet construction to enforce a maximum byte length using MAX_MEMORY_CONTEXT_SNIPPET_BYTES before admitting any ref. Keep the host-runtime boundary strict by rejecting oversized refs early, alongside the existing MEMORY_CONTEXT_REF_PREFIX and character checks.Source: Coding guidelines
crates/ironclaw_memory/src/service.rs (3)
269-280: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim before validating
MemoryContextProfileId.Line 270-Line 274 still accept
" "as a “validated” profile id. That pushes a blank identifier across the host→provider boundary and turns a boundary error into a later lookup miss.Proposed fix
fn validate(value: &str) -> Result<(), MemoryServiceError> { - if value.is_empty() { + if value.trim().is_empty() { return Err(MemoryServiceError::input()); } Ok(()) }As per coding guidelines, “Keep value-type constructors validating at the boundary; do not add unchecked public constructors.”
🤖 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 269 - 280, `MemoryContextProfileId::validate` currently only rejects empty strings, so whitespace-only values like " " still pass and become invalid IDs later. Update the boundary validation in `MemoryContextProfileId::new`/`validate` to trim the incoming string before checking emptiness, and reject any value that is blank after trimming while keeping the constructor fully validating and unchecked-free.Source: Coding guidelines
202-209: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject non-string
memory_tree.pathinstead of listing scope root.Line 203-Line 207 treat
path: 123the same as a missing path, so a malformed request widens into a root tree listing for the whole authorized scope. Keep missing/null as root, but fail whenpathis present and not a string.Proposed fix
- let path = input - .get("path") - .and_then(Value::as_str) - .unwrap_or("") - .to_string(); + let path = match input.get("path") { + None | Some(Value::Null) => String::new(), + Some(Value::String(path)) => path.clone(), + Some(_) => return Err(MemoryServiceError::input()), + };As per path instructions, “Fail loud: flag silent-failure patterns.”
🤖 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 202 - 209, Reject non-string memory_tree.path values in from_tool_input instead of coercing them to an empty path. Update the input parsing in MemoryService::from_tool_input so missing or null path still maps to root, but a present path field that is not a string returns a MemoryServiceError rather than falling back to “”. Keep the existing depth handling unchanged and ensure the validation happens where path is read from the Value.Source: Path instructions
35-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail loud on malformed numeric tool fields.
Line 495-Line 497 collapse present-but-invalid
limit/depthvalues intoNone, so{"limit":"10"}or{"depth":-1}silently fall back to defaults instead of returning invalid input.Proposed fix
- let limit = optional_u64(input, "limit").unwrap_or(5).clamp(1, 20) as usize; + let limit = optional_u64(input, "limit")?.unwrap_or(5).clamp(1, 20) as usize; @@ - let depth = optional_u64(input, "depth").unwrap_or(1).clamp(1, 10) as usize; + let depth = optional_u64(input, "depth")?.unwrap_or(1).clamp(1, 10) as usize; @@ -fn optional_u64(input: &Value, key: &'static str) -> Option<u64> { - input.get(key).and_then(Value::as_u64) +fn optional_u64(input: &Value, key: &'static str) -> Result<Option<u64>, MemoryServiceError> { + match input.get(key) { + None | Some(Value::Null) => Ok(None), + Some(Value::Number(value)) => value + .as_u64() + .map(Some) + .ok_or_else(MemoryServiceError::input), + Some(_) => Err(MemoryServiceError::input()), + } }As per path instructions, “Fail loud: flag silent-failure patterns.”
Also applies to: 208-208, 495-497
🤖 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 35 - 36, The numeric tool-field parsing is silently treating malformed present values as missing, so invalid inputs like a string for limit or a negative depth fall back to defaults instead of failing. Update the parsing path used by `optional_u64` and the `Self { query, limit }` construction so it distinguishes “absent” from “present but invalid” and returns an error for malformed `limit`/`depth` values rather than converting them to `None` or defaulting.Source: Path instructions
crates/ironclaw_memory_native/CLAUDE.md (1)
8-8: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNarrow the ownership claim here to repository/filesystem seams.
This still says
ironclaw_memory_nativeowns the/memoryvirtual path grammar, butsrc/path.rsnow says the public scope/path types and validators moved toironclaw_memory. Leaving both statements in-tree gives contributors two conflicting module specs. Please narrow this guardrail and keep the crate rustdoc aligned in the same pass.As per path instructions, “Module specs win ties.”
🤖 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_native/CLAUDE.md` at line 8, Narrow the ownership statement in CLAUDE.md so it only claims repository/filesystem seams and remove any reference to owning the /memory virtual path grammar. Align the crate guidance with the public path/module ownership now defined in ironclaw_memory and src/path.rs, and update the nearby rustdoc/spec wording in the same pass so there is a single consistent module contract. Use the existing ironclaw_memory_native guardrail text and the path-related symbols as the anchor for the edit.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.
Inline comments:
In `@crates/ironclaw_memory/src/context.rs`:
- Around line 47-52: The public setter with_prompt_write_safety_allowance
currently lets backend code attach arbitrary PromptSafetyAllowanceId values,
including empty_prompt_file_clear(), which can bypass prompt-write safety. Move
this allowance assignment behind a host/native-minted token path, or add
validation at the host-mediated call site before Context receives it, so only
trusted host-generated allowances can be stored in
prompt_write_safety_allowance.
---
Outside diff comments:
In `@docs/reborn/contracts/memory.md`:
- Around line 5-20: Update the repo-wide memory ownership docs so they reflect
the new provider-neutral split: `ironclaw_memory` should be described only as
the contract surface, while `ironclaw_memory_native` owns the concrete
adapter/search/indexing/bootstrap responsibilities. Sweep the stale wording in
`storage-placement.md`, `filesystem.md`, `_contract-freeze-index.md`,
`crates/AGENTS.md`, and `crates/README.md`, and keep the language consistent
with the split note already introduced in `memory.md`. Refer to the existing
`MemoryService`/`ironclaw_memory_native` terminology so the docs stay aligned
with the repo-wide doc-hygiene rules.
---
Duplicate comments:
In `@crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs`:
- Around line 33-38: Authorize the `/memory` write before parsing any profile
input in `MemoryServiceProfileSetRequest` handling, so unauthorized callers fail
closed with `FilesystemDenied` instead of getting validation errors. Move the
`ensure_memory_mount(request, /* write */ true)?` check ahead of
`MemoryServiceProfileSetRequest::from_tool_input(&request.input)` in
`profile_set.rs`, keeping the existing error mapping and request flow intact.
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 23-26: The snippet admission path in memory_context.rs only
validates the provider-controlled snippet_ref prefix/characters, so a large ref
can still bypass the byte budgets and reach LoopContextSnippet. Update the
validation in the snippet handling flow around the snippet_ref check and
LoopContextSnippet construction to enforce a maximum byte length using
MAX_MEMORY_CONTEXT_SNIPPET_BYTES before admitting any ref. Keep the host-runtime
boundary strict by rejecting oversized refs early, alongside the existing
MEMORY_CONTEXT_REF_PREFIX and character checks.
In `@crates/ironclaw_memory_native/CLAUDE.md`:
- Line 8: Narrow the ownership statement in CLAUDE.md so it only claims
repository/filesystem seams and remove any reference to owning the /memory
virtual path grammar. Align the crate guidance with the public path/module
ownership now defined in ironclaw_memory and src/path.rs, and update the nearby
rustdoc/spec wording in the same pass so there is a single consistent module
contract. Use the existing ironclaw_memory_native guardrail text and the
path-related symbols as the anchor for the edit.
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 105-116: The search/tree DTO mapping in NativeMemoryService is
trusting backend output and can leak data across scopes. Re-apply the same scope
check used in retrieve_context before constructing MemoryServiceSearchResult and
the tree DTOs, filtering out any results whose result.path.scope() does not
match the current context.scope(). Keep the validation in the search and tree
paths that call backend.search/backend.tree so all public returns preserve
tenant/user/project scope.
- Around line 111-116: The memory response DTOs and bootstrap-clear messaging
are leaking protected internal filenames such as BOOTSTRAP.md and HEARTBEAT.md
through public outputs. Update the mappings in MemoryService::search and the
related bootstrap-clear flow to alias, redact, or suppress internal filenames
before serializing MemoryServiceSearchResult or user-facing messages, using the
existing service-layer transformation points rather than exposing
path.relative_path() or raw file names directly.
- Around line 111-116: The `search` mapping in `MemoryServiceSearchResult` is
forwarding raw backend scores without filtering, so non-finite values can break
JSON responses. Update the `search` flow alongside `retrieve_context` to drop or
skip results whose `result.score` is not finite before constructing
`MemoryServiceSearchResult`, using the existing
`search`/`MemoryServiceSearchResult` mapping as the place to apply the
hardening.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 269-280: `MemoryContextProfileId::validate` currently only rejects
empty strings, so whitespace-only values like " " still pass and become
invalid IDs later. Update the boundary validation in
`MemoryContextProfileId::new`/`validate` to trim the incoming string before
checking emptiness, and reject any value that is blank after trimming while
keeping the constructor fully validating and unchecked-free.
- Around line 202-209: Reject non-string memory_tree.path values in
from_tool_input instead of coercing them to an empty path. Update the input
parsing in MemoryService::from_tool_input so missing or null path still maps to
root, but a present path field that is not a string returns a MemoryServiceError
rather than falling back to “”. Keep the existing depth handling unchanged and
ensure the validation happens where path is read from the Value.
- Around line 35-36: The numeric tool-field parsing is silently treating
malformed present values as missing, so invalid inputs like a string for limit
or a negative depth fall back to defaults instead of failing. Update the parsing
path used by `optional_u64` and the `Self { query, limit }` construction so it
distinguishes “absent” from “present but invalid” and returns an error for
malformed `limit`/`depth` values rather than converting them to `None` or
defaulting.
In `@docs/reborn/contracts/memory.md`:
- Around line 12-32: The responsibilities list in the `ironclaw_memory` contract
section still mixes provider-neutral contract duties with concrete
implementation work. Update the bullets around the `MemoryService` split so
`ironclaw_memory` only describes shared contracts/value types and
provider-agnostic semantics, while repository seams,
chunking/indexing/embeddings/search, prompt-context assembly,
seeding/bootstrap/profile sync, and similar implementation-specific
responsibilities are attributed to `ironclaw_memory_native`. Use the existing
`ironclaw_memory` and `ironclaw_memory_native` split wording to keep the section
aligned with the new crate ownership.
🪄 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: bdd00917-0362-421d-9e48-21b0302028ed
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (52)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_event_projections/Cargo.tomlcrates/ironclaw_event_projections/tests/memory_prompt_safety_projection_contract.rscrates/ironclaw_event_projections/tests/memory_significant_events_projection_contract.rscrates/ironclaw_host_runtime/Cargo.tomlcrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/user_profile_source.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_memory/AGENTS.mdcrates/ironclaw_memory/CLAUDE.mdcrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/context.rscrates/ironclaw_memory/src/events.rscrates/ironclaw_memory/src/hash.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/metadata.rscrates/ironclaw_memory/src/path.rscrates/ironclaw_memory/src/safety.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory_native/AGENTS.mdcrates/ironclaw_memory_native/CLAUDE.mdcrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/backend.rscrates/ironclaw_memory_native/src/chunking.rscrates/ironclaw_memory_native/src/contract_tests.rscrates/ironclaw_memory_native/src/embedding.rscrates/ironclaw_memory_native/src/events.rscrates/ironclaw_memory_native/src/filesystem.rscrates/ironclaw_memory_native/src/indexer.rscrates/ironclaw_memory_native/src/lib.rscrates/ironclaw_memory_native/src/metadata.rscrates/ironclaw_memory_native/src/path.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/repo/in_memory.rscrates/ironclaw_memory_native/src/repo/mod.rscrates/ironclaw_memory_native/src/safety.rscrates/ironclaw_memory_native/src/schema.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/src/write_metadata.rscrates/ironclaw_memory_native/tests/memory_backend_contract.rscrates/ironclaw_memory_native/tests/memory_filesystem_contract.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_memory_native/tests/repo_filesystem_contract.rscrates/ironclaw_memory_native/tests/repo_in_memory_contract.rscrates/ironclaw_product_adapters/tests/product_adapter_contract.rsdocs/reborn/contracts/memory.mddocs/reborn/contracts/storage-placement.md
10 tasks
henrypark133
approved these changes
Jun 25, 2026
BenKurrek
added a commit
that referenced
this pull request
Jun 26, 2026
…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>
BenKurrek
added a commit
that referenced
this pull request
Jun 26, 2026
…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>
BenKurrek
added a commit
that referenced
this pull request
Jun 26, 2026
…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>
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>
7 tasks
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Lifts the Reborn memory layer out of the kernel into a provider-neutral contract crate (
ironclaw_memory) plus a native filesystem provider crate (ironclaw_memory_native), and routes the existing first-party memory tools through a provider-neutralMemoryServicefacade. Implements #3537 ("model memory as a userland extension").Strictly behavior-preserving — the memory tools' observable behavior (input acceptance/rejection, response JSON, error kinds, stored-document semantics, prompt-context retrieval) is identical to
origin/main. The native provider keeps the existing filesystem storage format. No new user-facing behavior.Structure
ironclaw_memory(agnostic contract) — theMemoryServicetrait + operation request/response DTOs, memory-document scope/path/context value types, prompt-write-safety vocabulary, and the significant-event/audit contracts. Among internal IronClaw crates it depends only onironclaw_host_api.ironclaw_memory_native(provider) —NativeMemoryService+ the filesystem backend,/memoryadapter, chunking/search/indexing, and the prompt-write-safety enforcement engine. Depends on the agnostic crate (never the reverse); governed by a newironclaw_architecturedependency-boundary rule and added toproduct_adapters' forbidden lower-layer list.memory_search/read/write/tree,profile_set) and prompt-context retrieval route throughArc<dyn MemoryService>.Behavior preservation
A behavior-equivalence audit against
origin/mainconfirms the memory tools' observable behavior is unchanged: lenient tool-input parsing, the bootstrap-clear response (BOOTSTRAP.md), search/tree result sets, thememory-snippet:*ref hashing, disabled-context-profile gating (host-side, fail-closed before any provider call), the prompt-context byte budget, and all surfaced error kinds match origin. The host serializers reproduce the original response JSON byte-for-byte; the status enums and context-profile-id newtype are type-only changes that serialize identically. Thememory.md/storage-placement.mdcontracts were updated to reflect the new crate ownership.Scope notes
Deliberately not included (out of scope — #3537 is purely architectural and mandates no memory-tool behavior change):
prompt_doc_refmanifest-enforcement rule, an unwired memory-profile binding resolver, and unwired memory host-port scaffolding (ripped out earlier);Registering the native provider as a distinct extension manifest and wiring provider selection is a follow-up milestone.
🤖 Generated with Claude Code