feat(memory): rebuild the memory provider contract around declared capabilities - #6724
Merged
Merged
Conversation
…ycle hooks The [memory] manifest section now declares lifecycle participation (read_long_term | read_short_term | record_interaction | profile_read) instead of the operation-family vocabulary. document_store is gone: the manifest's [[tools]] array is the document-store surface, so a separate enum variant meaning "assume four tools" was redundant. The parser no longer rejects an honest manifest that backs only a subset (F2), and an empty/absent lifecycle is legal (a tools-only backend). mem0's bundled manifest now declares the honest set: read_long_term + profile_read only (it has no thread partitioning and does not record interactions — F5 at the manifest level). Test-first: manifest_v3_contract memory section rewritten to pin the new contract (subset accepted, empty/absent lifecycle accepted, unknown token fail-closed); MemoryLifecycleHook wire tokens pinned in ironclaw_host_api. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MemoryService loses retrieve_context and the provided read_long_term / read_thread wrappers; providers now implement read_long_term and read_short_term directly, so lane semantics are an explicit provider capability rather than an undocumented thread_id convention only native honored (F4). Both lanes return RAW snippets: the whole prompt-safety pipeline — ExpectedScope cross-scope drop, control-strip + truncate + untrusted envelope, per-snippet cap, empty-on-error degradation — moves into the host call site (ironclaw_host_runtime::memory_context), which now queries both lane methods with the same thread-carrying invocation. Native's thread branch becomes two lane bodies over a shared ranked_in_scope_results helper: read_long_term excludes threads/ even when the invocation carries the active thread; read_short_term requires it and degrades to empty without one. mem0 implements read_long_term only (its namespace has no thread axis), so the short-term lane stays at the fail-closed default and the host no longer receives duplicate snippets from two identical queries — the F4 duplicate-budget-burn is gone. ironclaw_memory no longer depends on ironclaw_prompt_envelope. Test-first: memory_prompt_context (host caller tier) mock ported to the lane methods and pins that both lanes receive the full scope incl. thread; hostile/out-of-scope/oversized provider output is still dropped, enveloped and capped by the host. Native contract tests pin lane disjointness with a thread-carrying long-term invocation; mem0 pins read_short_term at the unavailable default with zero transport calls. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ating is clamped Each memory provider's manifest now declares its own [[tools]] surface. A [memory]-declaring manifest may declare tools under the reserved stable ironclaw.memory.* namespace even when its extension id differs (trust-safe: [memory] requires a first_party runtime, which requires a host-bundled source), so swapping the bound backend never renames the model's memory tools. mem0's manifest gains its real four document tools with the same schemas and gating posture as native — no longer a documentation artifact shaped like configuration (F1/F7 groundwork). Requested tool gating is requested, not granted: the v3 parser clamps a memory tool's declared origin_gate_matrix so Ungated survives only where the reviewed UNGATED_LOOP_RUN_CAPABILITIES allowlist grants it — a provider requesting an ungated write (or ungated Product/Automation) gets gated_unless_granted, mirroring how host trust policy clamps first_party_requested trust. Test-first: v3 contract tests pin the reserved-namespace allowance (and that it stays closed without [memory]), the write-tool clamp, and the allowlisted read keeping ungated; host_api pins the clamp matrix; a new first_party_tools test pins that a declared tool the bound provider cannot serve fails closed at dispatch with the model-visible operation error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cle on its declared hooks
The unconditional native_memory_first_party_package() insert is gone.
Composition now resolves the binding, loads THAT provider's manifest
bundle (resolve_memory_provider → ResolvedMemoryProvider{resolver,
package, lifecycle}), and registers its declared tools: native by
default, mem0's own package when mem0 is bound and constructible, and
NOTHING for Disabled or an unknown/unbuildable third party — tools are
absent from the model's surface instead of advertised-and-failing (F1/F7).
The registry provider allowlist, inline schema lane, and first-party
trust entries key off MEMORY_PROVIDER_PACKAGE_IDS instead of hardwiring
the native id.
Every lifecycle call site now consults the bound provider's declared
set, derived once by memory_lifecycle_consumers (shared by
build_reborn_runtime and the integration harness so the gating cannot
fork): ProductionMemoryPromptContextService queries only declared
retrieval lanes, the after-turn writer is not wired without
record_interaction, and the profile source falls back to Empty without
profile_read (F3/F8). The resolver drops the retired document_store
vocabulary (resolve_provider / with_third_party_provider).
Tests: host caller tier pins that an undeclared lane is never queried
and an empty lifecycle issues zero provider calls; composition pins the
consumer derivation and that Disabled/unknown bindings yield no package
and an empty lifecycle; integration group_memory gains
scenario_lifecycle_gates_host_memory_calls (recording provider, empty
vs full declaration across real turns, incl. the after-turn and
profile-read seams) and scenario_disabled_binding_offers_no_memory_tools
(zero ironclaw.memory tools in the captured model tool list, with a
positive control on the default group).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed by the provider manifest builtin.profile_set is gone: the profile-write tool now rides the bound memory provider's manifest as ironclaw.memory.profile_set ([[tools]] in both native and mem0), so a backend that cannot serve profile writes would simply not declare it. Its schema moves to the memory schema set (served inline like the other memory tools); the null-sentinel normalization keeps covering the renamed id; dispatch routes through the same MemoryCapabilityState. The reviewed posture survives the rename verbatim: the UNGATED_LOOP_RUN_CAPABILITIES entry, the builtin_capability_policy.toml grant (CapabilityMountProfile::Memory), and the approval-gate EXEMPTION (a private local agent-context write) all move to the new id — while builtin.trace_commons.profile_set (a public external write) stays deliberately non-exempt, pinned by the same local_dev_authorization tests. The origin-gate ratchet seed is updated as the reviewed diff it demands. F6 recorded decision (owner, 2026-07-27): profile-write atomicity stays per-provider — native CASes, mem0 read-merge-writes (no CAS) — accepted variance behind the shared tool id, documented in mem0's manifest. Tests: the exemption + grant + gate tests drive the renamed id; user_profile_roundtrip registers the memory package (where the tool now lives) with the production memory trust entry; the builtin package expectation lists drop profile_set; the Disabled-binding integration scenario asserts the profile tool is absent with the rest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ironclaw_memory (behind the test-support dev seam) now owns a provider-level contract suite in the contract_tests scaffolding style: factory closures, one #[tokio::test] per contract per impl via wiring macros. Contracts: scope isolation across tenant/user/agent/project; retrieval-lane disjointness under one thread-carrying invocation (F4's regression — mutation-verified to fail when native's long-term threads/ exclusion is removed); and the record_interaction round trip into the same thread's short-term lane, invisible to other threads (F5). Native wires the full suite over a fresh in-memory backing. mem0 wires the retrieval-only suite over a STATEFUL fake mem0 server that enforces the exact user_id namespace the real self-hosted server filters by, so the contract proves per-scope namespace derivation end to end rather than scripting responses. Per the recorded F6 decision there is deliberately no profile-CAS contract (per-provider atomicity variance is accepted and documented). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins MemoryOperationKind, retrieve_context, 'operations = [', builtin.profile_set, and document_store at zero occurrences across crates/ (including the WebUI frontend sources) and tests/integration/, in the reborn_retired_taxonomy style (path-scoped sanctions: the v1 gateway enclave and the gate itself). Mutation-verified: a planted term fails the gate with the offending path. The WebUI i18n tool-description key moves with the tool rename (tools.description.builtin.profile_set → tools.description.ironclaw.memory.profile_set) across all 11 locales, so the settings Tools tab keeps its localized description for the renamed capability. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6724
July 27, 2026 15:31
Destroyed
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces operation-based memory declarations with lifecycle hooks, adds manifest-driven memory tools and provider resolution, separates long- and short-term retrieval, centralizes host admission and authorization, and updates native/mem0 implementations, runtime wiring, schemas, localization, and integration coverage. ChangesMemory lifecycle contract
Host and composition wiring
Validation and surface migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
risk: medium
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| 0 | 0 | 0 | 1df5f0d4d36b |
Head: 1df5f0d4d36bf80c0268211a819efc494855cb54
Next: Human review or validation is required before merging.
Run details
Status: Current
Needs human: no
Needs validation: yes
Summary
No concrete actionable defects found in static review. Runtime validation is required because Cargo is unavailable in this environment.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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_memory_retired_vocabulary.rs`:
- Around line 62-103: Update scan_dir to return a Result and propagate read_dir,
directory-entry enumeration, and read_to_string errors instead of skipping them;
preserve recursive scanning and hit collection on success. Update the calling
test to propagate or assert the scan result so any filesystem failure fails the
retired-vocabulary gate.
In `@crates/ironclaw_extensions/tests/manifest_v3_contract.rs`:
- Around line 1036-1039: Update the assertion in the parse_v3 validation test to
match the typed CapabilityIdNotPrefixed error variant instead of checking
whether the diagnostic string contains "provider-prefixed". Preserve the
existing expectation that parsing the non-memory manifest fails, while asserting
the specific structured error returned by parse_v3.
- Around line 888-893: Update the manifest baseline construction in the tests
around FULL_LIFECYCLE_LINE to interpolate or otherwise reuse that constant
instead of duplicating the lifecycle literal. Ensure the replace-based tests
memory_surface_accepts_a_lifecycle_subset,
memory_surface_accepts_empty_lifecycle, memory_surface_accepts_absent_lifecycle,
and memory_surface_accepts_unknown_lifecycle_token all modify the intended
full-lifecycle baseline.
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 396-400: Extend the sanitize_truncates_long_text test for
sanitize_snippet_text with a multibyte UTF-8 input that exceeds
MAX_MEMORY_CONTEXT_SNIPPET_BYTES. Assert the result remains within the byte cap
and is valid UTF-8, exercising truncate_to_char_boundary rather than only the
ASCII path.
In `@crates/ironclaw_host_runtime/src/memory_native_extension.rs`:
- Around line 154-163: Update the bundled manifest loading logic around
`record.resolved().memory` to reject manifests without a `[memory]` section
instead of using `unwrap_or_default()`. Propagate a descriptive validation error
so the provider is not registered with an empty lifecycle; only retain a
fallback if it is explicitly justified with an inline `// silent-ok: <reason>`
comment.
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 444-467: Preserve the validation error from
MemorySearchRequest::new in the request-building flow instead of discarding it
with map_err(|_| ...). Update the error conversion to retain or attach the
underlying cause while still returning MemoryServiceError for this method; leave
the existing search, backend search, and result filtering behavior unchanged.
In `@crates/ironclaw_reborn_composition/src/local_dev_authorization/tests.rs`:
- Around line 509-518: Replace the synthetic authorization check in
local_dev_memory_profile_set_skips_approval_gate with a test that invokes the
actual ironclaw.memory.profile_set capability caller through dispatch. Assert
that this real call returns an Allow result, exercising the bound memory
provider’s manifest and grant resolution instead of
trace_commons_authorize_decision.
In `@crates/ironclaw_reborn_composition/src/memory_provider_factory.rs`:
- Around line 271-299: The ThirdParty arm in the memory provider resolution flow
hardcodes the mem0 extension ID when selecting a bundled provider package.
Replace this ID-specific branch with
memory_extension::provider_bundle_for(extension_id), handling its optional
result and propagating bundle validation errors while preserving the existing
resolver/package/lifecycle construction and fail-closed unbound behavior for
None or unavailable providers.
In `@crates/ironclaw_reborn_config/src/config_file.rs`:
- Around line 2493-2498: Rename the test function
memory_rejects_unknown_binding_key to reflect that it validates an unknown key
directly under the [memory] section, such as memory_rejects_unknown_section_key;
leave the fixture and assertions unchanged.
In
`@tests/integration/group_memory/scenario_lifecycle_gates_host_memory_calls.rs`:
- Around line 98-119: Update the empty-lifecycle assertions in the scenario test
to await the deterministic lifecycle-task drain/completion signal before
checking any zero-call counters. Use the existing completion mechanism
referenced by the later post-terminal record_interaction checks, and apply it
before assertions for both lifecycle arms; do not use a fixed delay.
In `@tests/integration/support/assertions.rs`:
- Around line 216-218: Move the tool-assertion documentation so it remains
directly above assert_model_tool_offered, and restore the original
prompt-assertion documentation directly above assert_system_prompt_contains.
Ensure each method has only its corresponding docs and preserve their existing
behavior.
In `@tests/integration/support/group_constructors.rs`:
- Around line 63-68: Run the disabled-memory scenario through the flat harness
builder instead of creating a RebornIntegrationGroup, since it uses only one
isolated thread. Add the builtin-tools-without-memory option to the flat
builder, update all consumers of builtin_tools_without_memory to use it
directly, and remove the unnecessary group constructor and coordinator setup.
In `@tests/integration/support/group_options.rs`:
- Around line 52-64: Move the Rustdoc block describing installation of the
in-memory InMemoryTurnEventSink back above with_turn_event_sink, and leave
with_bound_memory_provider documented only by its memory lifecycle contract.
Ensure each method’s documentation describes only its own behavior.
In `@tests/integration/support/group.rs`:
- Around line 1090-1104: Update the group setup to derive one effective profile
source from the production lifecycle decision in memory_lifecycle_consumers: use
an empty source when a bound provider does not declare ProfileRead, otherwise
retain the selected user_profile_source. Reuse that same Arc for both the
user_profile_source field and the GroupSharedStorage wiring, ensuring
user_profile_source_for_test() is the runtime source and undeclared lifecycle
hooks are never called.
In `@tests/integration/support/harness/profiles/core_builtin.rs`:
- Around line 182-200: Ensure without_memory_package() is honored for every
egress mode, not only the branch controlled by include_memory_package. Update
the runtime construction flow around local_dev_host_runtime_with_http_egress and
local_dev_host_runtime_with_registry_and_runtime_http_egress to use a
builtin-only registry for Live and RealPipeline as well, or explicitly reject
that combination. Add a regression test covering without_memory_package() with
those egress modes.
🪄 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: 1bad8296-d732-4743-a3b2-41fdfa6c21af
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (74)
Cargo.tomlcrates/ironclaw_architecture/tests/reborn_memory_retired_vocabulary.rscrates/ironclaw_architecture/tests/reborn_origin_gate_matrix_ratchet.rscrates/ironclaw_extensions/src/host_api/capability_provider.rscrates/ironclaw_extensions/src/lib.rscrates/ironclaw_extensions/src/v2.rscrates/ironclaw_extensions/src/v3.rscrates/ironclaw_extensions/tests/manifest_v3_contract.rscrates/ironclaw_host_api/src/capability.rscrates/ironclaw_host_api/src/memory.rscrates/ironclaw_host_runtime/assets/memory_mem0/manifest.tomlcrates/ironclaw_host_runtime/assets/memory_native/manifest.tomlcrates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/write.mdcrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/profile-set.input.v1.jsoncrates/ironclaw_host_runtime/assets/memory_native/schemas/memory/profile-set.output.v1.jsoncrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/ironclaw_host_runtime/src/memory_context.rscrates/ironclaw_host_runtime/src/memory_native_extension.rscrates/ironclaw_host_runtime/src/memory_provider.rscrates/ironclaw_host_runtime/src/surface.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/memory_prompt_context.rscrates/ironclaw_host_runtime/tests/user_profile_roundtrip.rscrates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory/src/service_contract_tests.rscrates/ironclaw_memory_mem0/Cargo.tomlcrates/ironclaw_memory_mem0/src/lib.rscrates/ironclaw_memory_mem0/src/service.rscrates/ironclaw_memory_mem0/tests/memory_service_contract.rscrates/ironclaw_memory_native/Cargo.tomlcrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/tests/memory_service_contract.rscrates/ironclaw_reborn_cli/Cargo.tomlcrates/ironclaw_reborn_composition/src/builtin_capability_policy.rscrates/ironclaw_reborn_composition/src/builtin_capability_policy.tomlcrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/local_dev_authorization/tests.rscrates/ironclaw_reborn_composition/src/memory_provider_factory.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/src/test_support/user_profile.rscrates/ironclaw_reborn_composition/tests/memory_mem0_swap.rscrates/ironclaw_reborn_config/src/config_file.rscrates/ironclaw_runner/src/runtime.rscrates/ironclaw_runner/src/turn_run_executor.rscrates/ironclaw_webui/frontend/src/i18n/ar.tscrates/ironclaw_webui/frontend/src/i18n/de.tscrates/ironclaw_webui/frontend/src/i18n/en.tscrates/ironclaw_webui/frontend/src/i18n/es.tscrates/ironclaw_webui/frontend/src/i18n/fr.tscrates/ironclaw_webui/frontend/src/i18n/hi.tscrates/ironclaw_webui/frontend/src/i18n/ja.tscrates/ironclaw_webui/frontend/src/i18n/ko.tscrates/ironclaw_webui/frontend/src/i18n/pt-BR.tscrates/ironclaw_webui/frontend/src/i18n/uk.tscrates/ironclaw_webui/frontend/src/i18n/zh-CN.tstests/integration/group_memory/main.rstests/integration/group_memory/scenario_disabled_binding_offers_no_memory_tools.rstests/integration/group_memory/scenario_lifecycle_gates_host_memory_calls.rstests/integration/group_memory/scenario_memory_search_finds_seeded.rstests/integration/profile.rstests/integration/support/assertions.rstests/integration/support/builder.rstests/integration/support/group.rstests/integration/support/group_constructors.rstests/integration/support/group_options.rstests/integration/support/harness/profiles/core_builtin.rstests/integration/support/harness/profiles/profile.rs
|
🚅 Deployed to the ironclaw-pr-6724 environment in ironclaw-ci-preview
|
This was referenced Jul 27, 2026
…party registry behind a host guard The stable memory contract shrinks to the four host-initiated lifecycle hooks (profile_read, read_long_term, read_short_term, record_interaction). The five tool methods leave the trait: provider tools are ordinary manifest-declared first-party capabilities. Composition registers the BOUND provider's pure-behavior handler via register_memory_tool_handler, which wraps it in the host-owned MemoryToolGuard — declared-id fail-closed, manifest-effects-derived /memory mount authority, schema-driven null-sentinel normalization, and the 1 MiB output bound — so family policy is enforced once at the registration seam, the same position runtime lanes hold for wasm/mcp tools. - ironclaw_memory: trait is lifecycle-only; the five ironclaw.memory.* id consts, shared DTO parsers, and wire serializers live here so output shapes cannot drift across backends. - Providers: the five tool operations become inherent methods (no logic change); native's handler sits beside its manifest in host_runtime, mem0's in composition (the only crate that may name mem0). - BuiltinFirstPartyTools sheds every memory arm; profile_set.rs is deleted; ResolvedMemoryProvider carries tool_handler, and an unbound/disabled binding registers neither surface nor dispatch. - Behavior note: mount authority is now checked before input validation uniformly (previously profile_set validated input first); observable only when both are missing. - Blessed stale artifacts of the committed profile_set rename that the vocabulary gate cannot scan: golden payload snapshots, the composition pub-use snapshot, and the struct test-support ratchet baseline (the memory.rs debt it froze is deleted). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6724
July 27, 2026 18:06
Destroyed
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/ironclaw_reborn_composition/src/memory_provider_factory.rs (1)
285-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThird-party provider selection still hardcodes the mem0 id — past review comment unaddressed.
resolve_memory_provider'sThirdPartyarm hardcodesMEM0_MEMORY_EXTENSION_IDto pick the bundle/tool-handler, same pattern flagged previously oncreate_third_party_provider. It's now duplicated in two places instead of fixed in one; adding a second third-party provider means editing this match again. Route through amemory_extension::provider_bundle_for(extension_id)keyed offMEMORY_PROVIDER_PACKAGE_IDSinstead.As per coding guidelines: "Use generic and extensible architectures rather than hardcoding specific integrations."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/memory_provider_factory.rs` around lines 285 - 329, Replace the hardcoded MEM0_MEMORY_EXTENSION_ID check in resolve_memory_provider’s ThirdParty arm with memory_extension::provider_bundle_for(extension_id), keyed by MEMORY_PROVIDER_PACKAGE_IDS. Use the returned bundle to construct the provider, lifecycle, package, and tool handler generically, preserving the existing fail-closed behavior for unsupported or unavailable providers.crates/ironclaw_memory_native/src/service.rs (1)
433-475: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDropped cause in new
ranked_in_scope_results— same pattern flagged in a prior review, still unresolved.
MemorySearchRequest::new(&request.query).map_err(|_| MemoryServiceError::input())?discards the validation error. This is a NEW shared helper (backs bothread_long_termandread_short_term), unlike the rest of this file which consistently preserves causes viaoperation_from/unavailable_from. A prior review on this same file already called out this exact discard pattern as Major and it was not carried forward into this refactor.🩹 Preserve the cause
- let search_request = MemorySearchRequest::new(&request.query) - .map_err(|_| MemoryServiceError::input())? + let search_request = MemorySearchRequest::new(&request.query) + .map_err(MemoryServiceError::operation_from)?As per coding guidelines, "Do not use
.map_err(|_| OtherError)when it discards the original cause... comments cannot exempt dropped causes."🤖 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 433 - 475, Preserve the validation error in ranked_in_scope_results when constructing MemorySearchRequest: replace the discarding map_err closure on MemorySearchRequest::new with the established error conversion that retains the original cause, such as the file’s existing operation_from pattern. Keep the existing input-error classification and subsequent search flow unchanged.
🤖 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 244-300: The five-arm memory-tool dispatch is duplicated between
serve_native_tool and Mem0MemoryToolHandler::dispatch. Extract the shared
parsing, service calls, response conversion, and unsupported-capability handling
into a single serve_memory_tool(service: &dyn MemoryService, request:
&FirstPartyCapabilityRequest) helper, then have both callers delegate to it
while preserving their existing service/provider resolution.
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 635-678: Update search_response_output to accept provider-specific
search provenance rather than hardcoding MEMORY_SEARCH_SCOPE and false for
external_services_searched. Pass the appropriate scope and external-service flag
from each provider or capability handler, including the mem0 dispatch path,
while preserving the existing output shape and native-memory values.
---
Duplicate comments:
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 433-475: Preserve the validation error in ranked_in_scope_results
when constructing MemorySearchRequest: replace the discarding map_err closure on
MemorySearchRequest::new with the established error conversion that retains the
original cause, such as the file’s existing operation_from pattern. Keep the
existing input-error classification and subsequent search flow unchanged.
In `@crates/ironclaw_reborn_composition/src/memory_provider_factory.rs`:
- Around line 285-329: Replace the hardcoded MEM0_MEMORY_EXTENSION_ID check in
resolve_memory_provider’s ThirdParty arm with
memory_extension::provider_bundle_for(extension_id), keyed by
MEMORY_PROVIDER_PACKAGE_IDS. Use the returned bundle to construct the provider,
lifecycle, package, and tool handler generically, preserving the existing
fail-closed behavior for unsupported or unavailable providers.
🪄 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: c22e5790-9e8e-490f-a182-74d3774034dc
⛔ Files ignored due to path filters (3)
tests/snapshots/golden_payload__context_surfacing.snapis excluded by!**/*.snap,!tests/snapshots/**tests/snapshots/golden_payload__parallel_tool_calls.snapis excluded by!**/*.snap,!tests/snapshots/**tests/snapshots/golden_payload__tool_call.snapis excluded by!**/*.snap,!tests/snapshots/**
📒 Files selected for processing (22)
crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rscrates/ironclaw_host_runtime/src/first_party.rscrates/ironclaw_host_runtime/src/first_party_tools/memory.rscrates/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/ironclaw_host_runtime/src/first_party_tools/profile_set.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/src/memory_provider.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_host_runtime/tests/user_profile_roundtrip.rscrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/service.rscrates/ironclaw_memory/src/service_contract_tests.rscrates/ironclaw_memory_mem0/src/service.rscrates/ironclaw_memory_mem0/tests/memory_service_contract.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/tests/memory_service_contract.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/memory_provider_factory.rscrates/ironclaw_reborn_composition/tests/memory_mem0_swap.rscrates/ironclaw_runner/tests/loop_driver_host.rsdocs/plans/composition-pubuse.snapshottests/integration/support/harness/assembly.rs
💤 Files with no reviewable changes (2)
- crates/ironclaw_host_runtime/src/first_party_tools/profile_set.rs
- crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.66% — 310782 / 362823 lines Per-crate breakdown (60 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
…l test_support module `src/test_support.rs` is the repo-wide convention for a feature-gated test-only module (the shape scripts/check_no_panics.py exempts); `src/service_contract_tests.rs` was the same zero-bytes-in-production module under a name the no-panics gate cannot recognize, so CI flagged its fixture `.expect()`s as production panics. Rename only — module contents, macros, and the two provider wirings are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6724
July 27, 2026 18:51
Destroyed
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Cargo.toml`:
- Around line 34-37: Update the Cargo feature comment for test-support to
explicitly identify it as a dev-only test-support seam and state that it exposes
the MemoryService contract suite to downstream provider integration tests, while
preserving the note that production builds contain zero bytes.
🪄 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: d895eead-1ec1-478e-ab29-eccaf342ccda
📒 Files selected for processing (4)
crates/ironclaw_memory/Cargo.tomlcrates/ironclaw_memory/src/lib.rscrates/ironclaw_memory/src/test_support.rscrates/ironclaw_memory_mem0/tests/memory_service_contract.rs
Open
6 tasks
…preserving errors, harness parity) Review fixes, most-severe first: - group harness: resolve ONE effective user-profile source and wire the SAME Arc into the runtime parts and GroupSharedStorage — a bound provider without ProfileRead now degrades to EmptyUserProfileSource exactly like production runtime.rs, instead of silently falling back to the local-dev filesystem source (and the test accessor now returns what the runtime actually uses). - local_dev_authorization: the profile_set exemption test now authorizes the MANIFEST-projected descriptor (via the registry projection) with grants resolved for the memory provider id, instead of a synthetic builtin-provider descriptor. - lifecycle-gating scenario: bounded negative-poll window before the zero-call assertions, so a mis-wired post-terminal record (which runs inline on the scheduler worker) fails the test instead of landing after the asserts. - memory_native_extension: a bundled provider manifest without [memory] fails loud (was unwrap_or_default => silently empty lifecycle); named regression test. - retired-vocabulary gate: scan I/O errors now fail the gate instead of being skipped. - MemoryServiceError::input_from preserves the MemorySearchRequest validation cause at both native call sites (new lane + its search twin). - core_builtin harness: without_memory_package() with Live/RealPipeline egress now errors instead of being silently ignored. - manifest_v3_contract: replace-based lifecycle tests go through a helper that asserts the needle still matches the baseline (a drifted needle was a silent no-op). - memory_context: multibyte truncation test (exercises the char-boundary walk-back). - doc/name hygiene: misattached Rustdoc on assert_system_prompt_contains and with_turn_event_sink restored; memory_rejects_unknown_section_key rename; ironclaw_memory test-support feature comment names its bar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6724
July 28, 2026 14:23
Destroyed
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…bilities # Conflicts: # crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_reborn_composition/src/factory.rs (1)
4618-4619: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly publish the memory package when its handler is usable.
The extension registry receives
resolved_memory.packageunconditionally, while handler registration below requires bothpackageandtool_handler. An unbuildable provider can therefore expose manifest-declared memory tools that have no dispatch handler. Derive the registry package from the same(package, tool_handler)pair and add the unbuildable-binding regression case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/factory.rs` around lines 4618 - 4619, Update the extension-registry setup in the surrounding factory flow to pass a memory package only when its corresponding tool_handler is also usable, matching the handler-registration condition below; otherwise pass no package. Add a regression test covering an unbuildable memory binding and verify its manifest-declared tools are not published without a dispatch handler.crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs (1)
73-96: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit the unrelated sandbox lint-baseline expansion.
These entries ratchet in credential-firewall and sandbox-attribution exceptions (
W5/W6/W8), unrelated to the memory-provider migration. Keep this PR focused so the memory contract change does not also accept unrelated production lint debt.
crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs#L73-L96: remove or move the sandboxdead-codebaseline entries.crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs#L259-L282: remove or move the matching sandboxtest-supportbaseline entries.As per coding guidelines, “Keep pull requests focused and avoid mixing unrelated concerns.”
🤖 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_struct_test_support_ratchet.rs` around lines 73 - 96, The sandbox dead-code and test-support baseline entries are unrelated to the memory-provider migration. In crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs lines 73-96, remove or move the sandbox FrozenPathCount entries; likewise remove or move the matching sandbox test-support baseline entries at lines 259-282, keeping these lint-baseline changes out of this PR.Source: Coding guidelines
crates/ironclaw_host_runtime/src/lib.rs (1)
691-696: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not downgrade terminal failures to model-visible errors.
FailureKind::Cancelledprojects toFailureFate::Terminal, but this branch returnsModelVisibleToolError. Any cancellation that reaches this mapper can continue the run and consume a model turn instead of terminating. Add a terminal disposition (and handle it at the executor boundary), or make construction of terminalRuntimeCapabilityFailures impossible with an enforced typed 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/lib.rs` around lines 691 - 696, The capability_failure_disposition function currently maps FailureFate::Terminal to ModelVisibleToolError, allowing cancellations such as FailureKind::Cancelled to continue execution. Add a distinct terminal disposition and handle it at the executor boundary, or enforce a typed boundary that prevents constructing terminal RuntimeCapabilityFailures; ensure terminal failures terminate the run rather than consuming another model turn.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.
Outside diff comments:
In `@crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs`:
- Around line 73-96: The sandbox dead-code and test-support baseline entries are
unrelated to the memory-provider migration. In
crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs lines
73-96, remove or move the sandbox FrozenPathCount entries; likewise remove or
move the matching sandbox test-support baseline entries at lines 259-282,
keeping these lint-baseline changes out of this PR.
In `@crates/ironclaw_host_runtime/src/lib.rs`:
- Around line 691-696: The capability_failure_disposition function currently
maps FailureFate::Terminal to ModelVisibleToolError, allowing cancellations such
as FailureKind::Cancelled to continue execution. Add a distinct terminal
disposition and handle it at the executor boundary, or enforce a typed boundary
that prevents constructing terminal RuntimeCapabilityFailures; ensure terminal
failures terminate the run rather than consuming another model turn.
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 4618-4619: Update the extension-registry setup in the surrounding
factory flow to pass a memory package only when its corresponding tool_handler
is also usable, matching the handler-registration condition below; otherwise
pass no package. Add a regression test covering an unbuildable memory binding
and verify its manifest-declared tools are not published without a dispatch
handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e2a3424-2a1b-4be7-9888-b06b8fd3dbcc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (10)
crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rscrates/ironclaw_host_runtime/src/lib.rscrates/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/ironclaw_reborn_composition/src/builtin_capability_policy.rscrates/ironclaw_reborn_composition/src/builtin_capability_policy.tomlcrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_runner/tests/loop_driver_host.rstests/integration/support/builder.rstests/integration/support/group.rs
kirikov
added a commit
to kirikov/ironclaw
that referenced
this pull request
Jul 31, 2026
…pstream main sync Squash of the staging-verified deploy/ironclaw2 tip (37 commits) onto the previously deployed one, kept as a single commit so a prod rollback is one revert. Contents: - Per-user discovered hosted-MCP catalogs (fork PR #7): ScopedPackageOverlay keyed by (tenant, user, thread); turn-start discovery under the CALLER's credential; surface/grants/trust/dispatch/egress read one overlaid view with global-registry fallback. Fixes the global-catalog clobber (nearai#6778): a worker turn can no longer overwrite the concierge's tool surface and vice versa. - Overlay fixes found on staging: vendor-recipe (ProductAuthAccount) credentials qualify for discovery; credential staging routed by requirement source; discovery/staging under the raw run scope. - Upstream nearai/main sync riding along as the branch base (memory provider contract nearai#6724, extension behavior restore nearai#6737, webui/LLM fixes, e2e-test work). No database migrations. Verified end-to-end on staging against the production marketplace code: concierge catalog 8 tools under its own user, worker catalog 2 tools under the hire user, dispatch delivered, no cross-principal clobber.
30 tasks done
20 of 28 tasks
l3ocifer
pushed a commit
to l3ocifer/frick-ironclaw
that referenced
this pull request
Sep 3, 2026
…pabilities (nearai#6724) * feat(memory): replace [memory] operation families with declared lifecycle hooks The [memory] manifest section now declares lifecycle participation (read_long_term | read_short_term | record_interaction | profile_read) instead of the operation-family vocabulary. document_store is gone: the manifest's [[tools]] array is the document-store surface, so a separate enum variant meaning "assume four tools" was redundant. The parser no longer rejects an honest manifest that backs only a subset (F2), and an empty/absent lifecycle is legal (a tools-only backend). mem0's bundled manifest now declares the honest set: read_long_term + profile_read only (it has no thread partitioning and does not record interactions — F5 at the manifest level). Test-first: manifest_v3_contract memory section rewritten to pin the new contract (subset accepted, empty/absent lifecycle accepted, unknown token fail-closed); MemoryLifecycleHook wire tokens pinned in ironclaw_host_api. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(memory): split retrieve_context into explicit provider lane methods MemoryService loses retrieve_context and the provided read_long_term / read_thread wrappers; providers now implement read_long_term and read_short_term directly, so lane semantics are an explicit provider capability rather than an undocumented thread_id convention only native honored (F4). Both lanes return RAW snippets: the whole prompt-safety pipeline — ExpectedScope cross-scope drop, control-strip + truncate + untrusted envelope, per-snippet cap, empty-on-error degradation — moves into the host call site (ironclaw_host_runtime::memory_context), which now queries both lane methods with the same thread-carrying invocation. Native's thread branch becomes two lane bodies over a shared ranked_in_scope_results helper: read_long_term excludes threads/ even when the invocation carries the active thread; read_short_term requires it and degrades to empty without one. mem0 implements read_long_term only (its namespace has no thread axis), so the short-term lane stays at the fail-closed default and the host no longer receives duplicate snippets from two identical queries — the F4 duplicate-budget-burn is gone. ironclaw_memory no longer depends on ironclaw_prompt_envelope. Test-first: memory_prompt_context (host caller tier) mock ported to the lane methods and pins that both lanes receive the full scope incl. thread; hostile/out-of-scope/oversized provider output is still dropped, enveloped and capped by the host. Native contract tests pin lane disjointness with a thread-carrying long-term invocation; mem0 pins read_short_term at the unavailable default with zero transport calls. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(memory): provider manifests declare their own tools; requested gating is clamped Each memory provider's manifest now declares its own [[tools]] surface. A [memory]-declaring manifest may declare tools under the reserved stable ironclaw.memory.* namespace even when its extension id differs (trust-safe: [memory] requires a first_party runtime, which requires a host-bundled source), so swapping the bound backend never renames the model's memory tools. mem0's manifest gains its real four document tools with the same schemas and gating posture as native — no longer a documentation artifact shaped like configuration (F1/F7 groundwork). Requested tool gating is requested, not granted: the v3 parser clamps a memory tool's declared origin_gate_matrix so Ungated survives only where the reviewed UNGATED_LOOP_RUN_CAPABILITIES allowlist grants it — a provider requesting an ungated write (or ungated Product/Automation) gets gated_unless_granted, mirroring how host trust policy clamps first_party_requested trust. Test-first: v3 contract tests pin the reserved-namespace allowance (and that it stays closed without [memory]), the write-tool clamp, and the allowlisted read keeping ungated; host_api pins the clamp matrix; a new first_party_tools test pins that a declared tool the bound provider cannot serve fails closed at dispatch with the model-visible operation error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(memory): register only the bound provider's package; gate lifecycle on its declared hooks The unconditional native_memory_first_party_package() insert is gone. Composition now resolves the binding, loads THAT provider's manifest bundle (resolve_memory_provider → ResolvedMemoryProvider{resolver, package, lifecycle}), and registers its declared tools: native by default, mem0's own package when mem0 is bound and constructible, and NOTHING for Disabled or an unknown/unbuildable third party — tools are absent from the model's surface instead of advertised-and-failing (F1/F7). The registry provider allowlist, inline schema lane, and first-party trust entries key off MEMORY_PROVIDER_PACKAGE_IDS instead of hardwiring the native id. Every lifecycle call site now consults the bound provider's declared set, derived once by memory_lifecycle_consumers (shared by build_reborn_runtime and the integration harness so the gating cannot fork): ProductionMemoryPromptContextService queries only declared retrieval lanes, the after-turn writer is not wired without record_interaction, and the profile source falls back to Empty without profile_read (F3/F8). The resolver drops the retired document_store vocabulary (resolve_provider / with_third_party_provider). Tests: host caller tier pins that an undeclared lane is never queried and an empty lifecycle issues zero provider calls; composition pins the consumer derivation and that Disabled/unknown bindings yield no package and an empty lifecycle; integration group_memory gains scenario_lifecycle_gates_host_memory_calls (recording provider, empty vs full declaration across real turns, incl. the after-turn and profile-read seams) and scenario_disabled_binding_offers_no_memory_tools (zero ironclaw.memory tools in the captured model tool list, with a positive control on the default group). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(memory): profile_set becomes ironclaw.memory.profile_set, declared by the provider manifest builtin.profile_set is gone: the profile-write tool now rides the bound memory provider's manifest as ironclaw.memory.profile_set ([[tools]] in both native and mem0), so a backend that cannot serve profile writes would simply not declare it. Its schema moves to the memory schema set (served inline like the other memory tools); the null-sentinel normalization keeps covering the renamed id; dispatch routes through the same MemoryCapabilityState. The reviewed posture survives the rename verbatim: the UNGATED_LOOP_RUN_CAPABILITIES entry, the builtin_capability_policy.toml grant (CapabilityMountProfile::Memory), and the approval-gate EXEMPTION (a private local agent-context write) all move to the new id — while builtin.trace_commons.profile_set (a public external write) stays deliberately non-exempt, pinned by the same local_dev_authorization tests. The origin-gate ratchet seed is updated as the reviewed diff it demands. F6 recorded decision (owner, 2026-07-27): profile-write atomicity stays per-provider — native CASes, mem0 read-merge-writes (no CAS) — accepted variance behind the shared tool id, documented in mem0's manifest. Tests: the exemption + grant + gate tests drive the renamed id; user_profile_roundtrip registers the memory package (where the tool now lives) with the production memory trust entry; the builtin package expectation lists drop profile_set; the Disabled-binding integration scenario asserts the profile tool is absent with the rest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(memory): shared MemoryService provider conformance suite ironclaw_memory (behind the test-support dev seam) now owns a provider-level contract suite in the contract_tests scaffolding style: factory closures, one #[tokio::test] per contract per impl via wiring macros. Contracts: scope isolation across tenant/user/agent/project; retrieval-lane disjointness under one thread-carrying invocation (F4's regression — mutation-verified to fail when native's long-term threads/ exclusion is removed); and the record_interaction round trip into the same thread's short-term lane, invisible to other threads (F5). Native wires the full suite over a fresh in-memory backing. mem0 wires the retrieval-only suite over a STATEFUL fake mem0 server that enforces the exact user_id namespace the real self-hosted server filters by, so the contract proves per-scope namespace derivation end to end rather than scripting responses. Per the recorded F6 decision there is deliberately no profile-CAS contract (per-provider atomicity variance is accepted and documented). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(architecture): zero-legacy gate for the retired memory vocabulary Pins MemoryOperationKind, retrieve_context, 'operations = [', builtin.profile_set, and document_store at zero occurrences across crates/ (including the WebUI frontend sources) and tests/integration/, in the reborn_retired_taxonomy style (path-scoped sanctions: the v1 gateway enclave and the gate itself). Mutation-verified: a planted term fails the gate with the offending path. The WebUI i18n tool-description key moves with the tool rename (tools.description.builtin.profile_set → tools.description.ironclaw.memory.profile_set) across all 11 locales, so the settings Tools tab keeps its localized description for the renamed capability. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(memory): lifecycle-only MemoryService; tools ride the first-party registry behind a host guard The stable memory contract shrinks to the four host-initiated lifecycle hooks (profile_read, read_long_term, read_short_term, record_interaction). The five tool methods leave the trait: provider tools are ordinary manifest-declared first-party capabilities. Composition registers the BOUND provider's pure-behavior handler via register_memory_tool_handler, which wraps it in the host-owned MemoryToolGuard — declared-id fail-closed, manifest-effects-derived /memory mount authority, schema-driven null-sentinel normalization, and the 1 MiB output bound — so family policy is enforced once at the registration seam, the same position runtime lanes hold for wasm/mcp tools. - ironclaw_memory: trait is lifecycle-only; the five ironclaw.memory.* id consts, shared DTO parsers, and wire serializers live here so output shapes cannot drift across backends. - Providers: the five tool operations become inherent methods (no logic change); native's handler sits beside its manifest in host_runtime, mem0's in composition (the only crate that may name mem0). - BuiltinFirstPartyTools sheds every memory arm; profile_set.rs is deleted; ResolvedMemoryProvider carries tool_handler, and an unbound/disabled binding registers neither surface nor dispatch. - Behavior note: mount authority is now checked before input validation uniformly (previously profile_set validated input first); observable only when both are missing. - Blessed stale artifacts of the committed profile_set rename that the vocabulary gate cannot scan: golden payload snapshots, the composition pub-use snapshot, and the struct test-support ratchet baseline (the memory.rs debt it froze is deleted). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(memory): move the provider conformance suite to the canonical test_support module `src/test_support.rs` is the repo-wide convention for a feature-gated test-only module (the shape scripts/check_no_panics.py exempts); `src/service_contract_tests.rs` was the same zero-bytes-in-production module under a name the no-panics gate cannot recognize, so CI flagged its fixture `.expect()`s as production panics. Rename only — module contents, macros, and the two provider wirings are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(memory): address CodeRabbit review round (fail-loud gates, cause-preserving errors, harness parity) Review fixes, most-severe first: - group harness: resolve ONE effective user-profile source and wire the SAME Arc into the runtime parts and GroupSharedStorage — a bound provider without ProfileRead now degrades to EmptyUserProfileSource exactly like production runtime.rs, instead of silently falling back to the local-dev filesystem source (and the test accessor now returns what the runtime actually uses). - local_dev_authorization: the profile_set exemption test now authorizes the MANIFEST-projected descriptor (via the registry projection) with grants resolved for the memory provider id, instead of a synthetic builtin-provider descriptor. - lifecycle-gating scenario: bounded negative-poll window before the zero-call assertions, so a mis-wired post-terminal record (which runs inline on the scheduler worker) fails the test instead of landing after the asserts. - memory_native_extension: a bundled provider manifest without [memory] fails loud (was unwrap_or_default => silently empty lifecycle); named regression test. - retired-vocabulary gate: scan I/O errors now fail the gate instead of being skipped. - MemoryServiceError::input_from preserves the MemorySearchRequest validation cause at both native call sites (new lane + its search twin). - core_builtin harness: without_memory_package() with Live/RealPipeline egress now errors instead of being silently ignored. - manifest_v3_contract: replace-based lifecycle tests go through a helper that asserts the needle still matches the baseline (a drifted needle was a silent no-op). - memory_context: multibyte truncation test (exercises the char-boundary walk-back). - doc/name hygiene: misattached Rustdoc on assert_system_prompt_contains and with_turn_event_sink restored; memory_rejects_unknown_section_key rename; ironclaw_memory test-support feature comment names its bar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This branch is being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(memory): rebuild the memory provider contract around declared capabilities
Branch
memory/lifecycle-capabilities(8 commits, one per phase, test-first inside each).The bound provider's manifest is now the single source of truth for the memory
surface: its
[[tools]]array is what the model sees, and its[memory].lifecycleset is the only thing the agent loop's host-initiated memory calls consult. A hook
that is not declared is never called; a binding that yields no provider registers
no memory tools at all.
The stable memory contract is lifecycle-only:
MemoryServiceis exactly thefour host-initiated hooks. Tools are not part of any memory contract — they are
ordinary manifest-declared first-party capabilities, routed by the existing
FirstPartyCapabilityRegistry, with one host-owned guard applied atregistration. A future provider can declare any tool ids it wants (its own
extension prefix, or the reserved
ironclaw.memory.*namespace when it wantsthe stable swap-safe names) without touching any shared memory code.
The new manifest contract
Lifecycle vocabulary (exactly four wire tokens,
MemoryLifecycleHook):read_long_term|read_short_term|record_interaction|profile_read.MemoryOperationKind/operations = [...]/document_storeare deleted;the parser no longer rejects an honest subset manifest (old F2), and
[memory]still requires afirst_partyruntime (host-bundled only).The contract: lifecycle-only
MemoryServiceMemoryServiceis now exactly the four hooks the host initiates:profile_read(loop start) ·read_long_term/read_short_term(once perrun at first prompt build) ·
record_interaction(after every completed run).Defaults fail closed (
unavailable;record_interactiondefaults torecorded: false), so a provider implements only what it declares.MemoryService::retrieve_context(and the provided wrappers) are gone.Providers implement the lanes directly — lane semantics are a declared
capability, not a
thread_idconvention only native honored. Both lanes returnraw snippets; the entire prompt-safety pipeline (ExpectedScope cross-scope
drop, control-strip + truncate + untrusted envelope, 512 B per-snippet / 4 KiB
aggregate budgets, short-term-first ordering, empty-on-error degradation) lives
host-side in
ironclaw_host_runtime::memory_context, which queries only thedeclared lanes. Providers never return prompt-ready text — pinned by the
caller-tier
memory_prompt_contextsuite (hostile / out-of-scope / oversizedprovider output is still dropped, enveloped and capped).
Tools: registry-routed, one host-owned guard
The five tool methods left the trait entirely (Phase 8). A provider's tools are
ordinary first-party capabilities: the manifest declares them, and composition
registers the bound provider's handler with
register_memory_tool_handler(registry, package, handler). That function wrapsthe provider handler in the host-owned
MemoryToolGuardand registers itfor exactly the declared ids — registration is the only way a memory tool
becomes dispatchable, so the guard is unforgeable and never duplicated
per provider:
/memorymount check, requiring write permissionexactly when the tool's manifest-declared effects include a write (derived,
not hardcoded — this also unified the previous per-tool ordering quirk:
mount authority is now checked before input validation uniformly);
declared input schema;
Provider handlers are pure behavior. Native's (
NativeMemoryToolHandler,beside its manifest in
ironclaw_host_runtime) builds the native service overeach request's filesystem with the audit-backed prompt-write-safety sink;
mem0's lives in composition — the only crate allowed to name mem0 — holding the
concrete service built at startup. Both parse with the shared DTO parsers
(which carry the traversal/out-of-scope input guards) and serialize with the
shared wire helpers in
ironclaw_memory, so output shapes cannot drift acrossbackends. The old Rust-enumerated memory arms in
BuiltinFirstPartyTools(andprofile_set.rs) are deleted — the host no longer enumerates memory tool idsanywhere.
Wiring
factory.rs's unconditionalnative_memory_first_party_package()insert isgone. Composition resolves the binding once
(
resolve_memory_provider→ResolvedMemoryProvider { resolver, package, lifecycle, tool_handler })and registers the bound provider's package + guarded tool handler: native
by default, mem0's own when bound + constructible, and nothing for
Disabledor an unknown/unbuildable third party — tools are absent from themodel surface AND from dispatch instead of advertised-and-failing.
(
memory_lifecycle_consumers, used bybuild_reborn_runtimeand theintegration harness so the gate cannot fork): the prompt-context adapter
queries only declared lanes; the after-turn writer is unwired without
record_interaction; the profile source falls back to Empty withoutprofile_read.builtin.profile_set→ironclaw.memory.profile_set, declared by the providermanifests. The reviewed posture moved verbatim: Ungated allowlist entry,
CapabilityMountProfile::Memorygrant, and the approval-gate exemption(private local write) — while
builtin.trace_commons.profile_set(publicexternal write) stays deliberately non-exempt, pinned by the same
local_dev_authorizationtests. WebUI i18n description keys moved across all11 locales.
reject
memory.disabled+ unverified third parties without a scoped override),the composition-only provider-crate dependency rule, the once-per-run OnceCell
retrieval cache, the reserved
threads/namespace, and the[memory]configsection shape (
provider/admin_overrides/mem0_base_url).F6 — profile-write atomicity (recorded decision)
Owner decision (Ben, 2026-07-27): per-provider variance is accepted — "the
internal system should not care." Native
profile_setcompare-and-swaps withretries; mem0 has no CAS and read-merge-writes (closes the field-drop gap;
concurrent writers can still race). The divergence is documented in mem0's
manifest next to its
profile_setdeclaration, and the Phase 6 conformancesuite deliberately carries no CAS contract.
Per-provider surface
ironclaw.memorymem0.local.memoryama.agent.memory(theirs)mem0's honest lifecycle is the F4/F5 fix in manifest form: no short-term lane
(its namespace has no thread axis — the host no longer issues two identical
queries whose duplicate results burned the 4 KiB budget), and no
record_interaction(the after-turn seam is skipped instead of no-opping everyturn behind a declared
interaction_log).Defect → named regression test
memory_mem0_swap::config_binding_swaps_the_memory_provider_to_mem0_through_the_factory(mem0's package + honest lifecycle come from its parsed manifest);resolve_memory_provider_native_binds_package_and_lifecyclemanifest_v3_contract::memory_surface_accepts_a_lifecycle_subsetmemory_prompt_context::{undeclared_short_term_lane_is_not_queried, empty_lifecycle_issues_no_retrieval_queries};memory_provider_factory::lifecycle_consumers_wire_only_declared_hooks; integrationgroup_memory::scenario_lifecycle_gates_host_memory_calls(recording provider, empty vs full declaration across real turns incl. after-turn + profile-read)retrieval_lanes_are_disjoint(mutation-verified RED); nativenative_context_retrieve_excludes_thread_scratch_from_long_term(thread-carrying long-term invocation); mem0read_short_term_stays_at_the_unavailable_default(zero transport calls)record_interaction_round_trips_into_retrieval; mem0's manifest no longer declares the hookgroup_memory::scenario_disabled_binding_offers_no_memory_tools(zeroironclaw__memory__*in the captured model tool list, positive control on the default group)first_party_tools::memorysuite through the guard→native chain:undeclared_capability_id_fails_closed, mount-authority + permission rejections, null-sentinel normalization, output-bound enforcement; mem0 swap test dispatches throughregister_memory_tool_handler(guard in path)reborn_memory_retired_vocabularygate:MemoryOperationKind,retrieve_context,operations = [,builtin.profile_set,document_storeat zero across crates/ + frontend + tests/integration (mutation-verified)Commits (one per phase)
feat(memory): replace [memory] operation families with declared lifecycle hooksfeat(memory): split retrieve_context into explicit provider lane methodsfeat(memory): provider manifests declare their own tools; requested gating is clampedfeat(memory): register only the bound provider's package; gate lifecycle on its declared hooksfeat(memory): profile_set becomes ironclaw.memory.profile_set, declared by the provider manifesttest(memory): shared MemoryService provider conformance suitetest(architecture): zero-legacy gate for the retired memory vocabularyrefactor(memory): lifecycle-only MemoryService; tools ride the first-party registry behind a host guardNotes for reviewers
ironclaw.memory.*tool ids on a foreignextension id) exists at both validation layers (v3 parse + descriptor
projection) and is trust-bounded both times:
[memory]⇒first_partyruntime ⇒ HostBundled source.
UNGATED_LOOP_RUN_CAPABILITIESdiff (S5 ratchet, reviewed):builtin.profile_set→ironclaw.memory.profile_set— a rename, not anaddition;
ironclaw.memory.writestays off the list.validation for all five tools (previously
profile_setvalidated inputfirst). Observable only in the double-failure corner (bad input + missing
mount now reports the mount error).
rename, which their formats hide from the vocabulary gate: the golden
payload
.snapfiles (builtin.profile_set→ironclaw.memory.profile_set,15↔15 + surface sha),
docs/plans/composition-pubuse.snapshot(theintentional new factory exports), and the struct test-support ratchet
baseline (memory.rs debt deleted by the rewrite — entries shrunk, the
direction the ratchet demands).
ironclaw --test smoke onboard_login_link_then_bearer_authorizes_a_protected_request;ironclaw_triggers repository_contract::libsql_repository_fire_claim_contract(busy_timeout race). Docker-less machines additionally fail loud (by REL-3
design) on the 3
sandbox_processlib tests and the 6StorageMode::Postgresrstest cases across backend_matrix / extension_delivery / extension_ingress /
extension_runtime — environmental; CI has Docker. The
ironclaw_llmfault-injection doctest referencing the retired root
ironclawcrate isbroken on main (crate untouched by this branch).
🤖 Generated with Claude Code
Closes #6730