feat(memory): host-managed memory lifecycle — two-lane retrieval + after-turn record (mem0 flow)

[BenKurrek]

Summary

Implements the mem0 host-managed memory flow on top of #5205 (reborn/memory-lift-followups). Memory now reaches the model proactively (the loop previously hardcoded memory_snippets: Vec::new()), and each completed turn is recorded back so the short-term lane is populated next time.

The retrieval + safety logic is self-contained in the memory service: two provider-agnostic methods own both lane-scoping and per-snippet sanitization, so no provider can return unsafe prompt context and the host is a thin adapter.

on_run_start:   long_term  = MemoryService::read_long_term(user scope)   // clears thread
                short_term = MemoryService::read_thread(thread scope)    // keeps thread
before_model:   prompt = system + long_term + short_term + conversation
after_turn:     MemoryService::record_interaction(full transcript, turn_run_id, metadata)

Architecture

Retrieval — on MemoryService (provider-agnostic defaults)

• read_long_term clears the thread sub-scope (without_thread_and_mission) → the user's general memory; read_thread keeps it → the active thread's scratch. Both are default trait methods that own the per-snippet safety: scope-check (drop cross-tenant/user/agent/project), control-char strip, size-cap, and the untrusted-memory envelope. Providers implement only the raw retrieve_context; they inherit safe lane reads, so a swapped/third-party provider cannot return unsafe context. (ironclaw_memory gains a leaf dep on ironclaw_prompt_envelope; no cycle.)
• Native retrieve_context does the FTS + the threads/<thread_id>/ lane filter, over-fetching before the filter so general hits in the global top-N can't starve the thread lane.
• ProductionMemoryPromptContextService is now a thin adapter: it makes the two scoped reads concurrently (tokio::join!), concatenates long-term then short-term (this conversation nearest the current message), and maps each safe snippet onto a LoopContextSnippet. The only logic left host-side is what depends on loop-layer types: the model-visible memory-snippet:* reference and the loop's prompt-content denylist (LoopSafeSummary) applied as a drop-filter, plus the combined 512 B/snippet + 4 KiB-aggregate budget.
• Fetched once per run (OnceCell); the query is the latest non-blank user message; a retrieval failure degrades to empty and is cached so a down/slow provider isn't re-hit every model step. Memory never breaks a turn.

Recording — record_interaction (the mem0 add seam; "provider decides")

• Low-level MemoryService::record_interaction(invocation, { messages, turn_run_id, metadata }) with a default no-op so providers opt in. The host passes the DATA — the full ordered turn transcript (every user / finalized-assistant / tool message), turn_run_id as provenance (not the mem0 session id — that's scope.thread_id), and opaque metadata — and the provider decides verbatim-store vs extraction vs nothing.
• The native provider writes the transcript to a per-run file threads/<thread_id>/<turn_run_id>.md with overwrite semantics (idempotent — a scheduler re-run of a Completed run overwrites, no duplication/unbounded growth). The threads/ namespace is reserved and write-rejected for tool/caller writes (a stray write there would be a silent retrieval black hole); only the recorder writes there, via a private bypass.
• AfterTurnMemoryRecorder fires at turn_run_executor::apply_exit, gated on Completed, reading the exchange with the owner-rewritten thread scope. It's timeout-bounded so a slow/hung provider can't occupy the scheduler worker, and post-terminal best-effort — every error is debug!-logged and never fails the already-completed run.

Safety boundary

• Prompt-context sanitization is provider-agnostic and always-on, living on the read methods. The model-visible output and drop-logic were independently audited as a no-regression move from the previous host-side implementation (envelope, denylist, reference hash, scope-check, and budgets all byte-identical).
• The agent's memory tools (memory_search/read/write/tree) and the profile read are separate surfaces with their own paths and are unchanged.

Testing

Unit + caller-level across ironclaw_memory{,_native}, host_runtime, loop_support, turns, reborn:

• read_long_term/read_thread lane split + envelope (native facade); per-snippet sanitizer (control-strip / truncate / envelope) + scope-drop unit tests; threads/ write-rejection + reserved-namespace bypass; short-term over-fetch survives the lane filter; idempotent per-run record.
• Host two-lane fetch (long-term-first) + per-lane degradation + combined budget; once-per-run cache; latest-non-blank-user query; render-boundary lane-ordering; prompt rendering from memory_snippets.
• After-turn: full-ordered-transcript capture (incl. final assistant), verbatim content (no trim), threadless/no-run-id no-ops, a full-turn record through the executor, and a caller-level test driving build_default_planned_runtime so the writer wiring can't silently regress.
• cargo fmt + cargo clippy clean on the touched crates; downstream crates (mem0 / reborn / reborn_composition) compile; root test harness compiles. (3 sandbox_process tests fail locally only without Docker.)

Known items / follow-ups

• Production graph wires None — reads + writes resolve on the local-dev runtime path only; the production graph degrades to no memory (issue Reborn: wire production-graph composition for optional context sources (identity + profile) #5013, same as user_profile_source).
• Full-composition e2e (record in one run → surface in a later run's model request) not yet written; the individual links are unit + caller tested.
• Budget priority: the combined block is now long-term-first, so under aggregate-budget pressure the long-term lane wins (was short-term-first before the consolidation). Per-lane sub-budgets are a later refinement.
• read_profile is intentionally left as profile_read (raw doc → host parses into the loop-shaped UserProfileContext); folding it into a parsed read_profile would require moving the timezone/locale validation down and a memory-owned profile type.
• Phase 3 (on_run_end durable summary) intentionally deferred.

Design doc: docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)

• staging
• reborn-integration

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: adc13583-826f-46fd-bb4e-e23e55f159d5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR adds a host-managed memory lifecycle: dual-lane memory retrieval, prompt injection with per-run caching, after-turn transcript persistence, and runtime wiring for optional memory retrieval/writes across reborn execution paths.

Changes

Reborn memory lifecycle

Layer / File(s)	Summary
Memory contract and native store crates/ironclaw_memory/..., crates/ironclaw_memory_native/..., crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs, crates/ironclaw_memory_native/tests/memory_service_facade.rs	Adds interaction-recording types and record_interaction, splits memory reads into read_long_term/read_thread, and updates native storage for thread-scoped retrieval, reserved threads/ writes, and per-run transcript persistence.
Prompt cache and memory section crates/ironclaw_host_runtime/..., crates/ironclaw_loop_support/..., crates/ironclaw_turns/..., crates/ironclaw_reborn/tests/llm_gateway.rs	Switches host memory loading to concurrent lane reads with loop denylist admission, caches prompt memory once per run from the latest user message, and preserves host insertion order when rendering memory snippets.
After-turn recorder crates/ironclaw_reborn/src/after_turn_memory.rs, crates/ironclaw_reborn/src/turn_run_executor.rs, crates/ironclaw_reborn/tests/loop_driver_host.rs	Adds post-completion transcript building and best-effort recording under timeout, then verifies the recorded per-run document is written and discoverable.
Runtime wiring and defaults crates/ironclaw_reborn/src/loop_driver_host.rs, crates/ironclaw_reborn/src/runtime.rs, crates/ironclaw_reborn_composition/src/runtime.rs, tests/integration/*, tests/support/*, crates/*/tests/*	Adds optional memory wiring fields to planned runtime parts, resolves them from composition, and updates parity and harness tests to account for the new runtime shape.
Lifecycle design spec docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md	Adds the memory lifecycle design/spec and its implementation progress log.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ThreadBackedLoopContextPort
  participant ProductionMemoryPromptContextService
  participant MemoryService
  participant InstructionBundleBuilder
  ThreadBackedLoopContextPort->>ProductionMemoryPromptContextService: load_memory_snippets(request)
  ProductionMemoryPromptContextService->>MemoryService: read_thread / read_long_term
  ProductionMemoryPromptContextService-->>ThreadBackedLoopContextPort: admitted snippets
  ThreadBackedLoopContextPort->>InstructionBundleBuilder: load_loop_context(memory_snippets)
  InstructionBundleBuilder-->>ThreadBackedLoopContextPort: rendered memory section

Loading

sequenceDiagram
  participant RebornTurnRunExecutor
  participant AfterTurnMemoryRecorder
  participant SessionThreadService
  participant MemoryService
  RebornTurnRunExecutor->>AfterTurnMemoryRecorder: record_completed_run(state)
  AfterTurnMemoryRecorder->>SessionThreadService: read thread history
  SessionThreadService-->>AfterTurnMemoryRecorder: ordered run messages
  AfterTurnMemoryRecorder->>MemoryService: record_interaction(turn_run_id, transcript, metadata)
  MemoryService-->>AfterTurnMemoryRecorder: recorded / skipped

Loading

Possibly related issues

• [Tracking] Reborn personal memory & self-learning — full system #5260: Covers the same proactive retrieval and after-turn transcript recording flow.
• Wiring-parity guard: EXPECTED_PRODUCTION_SHAPE is hand-derived — add a production-side shape accessor #5641: Tracks the memory_context_service / after_turn_memory_writer runtime seam and parity coverage.
• reborn: memory prompt-context injection is unwired — ProductionMemoryPromptContextService never composed, memory_snippets always empty #5605: Matches the prompt-memory wiring work that makes memory_snippets non-empty.
• [TEST] Reborn: Add vertical-slice integration test suite #3067: Matches the vertical slice of Reborn memory lifecycle wiring and tests.

Possibly related PRs

• nearai/ironclaw#5163: Prior memory-service facade work this PR extends in both host and native paths.
• nearai/ironclaw#5642: Introduced the wiring-parity surface that this PR updates for the new runtime fields.
• nearai/ironclaw#4795: Also touches InstructionBundleBuilder::build, which this PR changes for memory ordering.

Suggested reviewers: serrrfirat, ilblackdragon

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	Core content is present, but the required template sections for change type, linked issue, validation, security, and runtime checklist are missing.	Add the missing template sections and fill in the approved issue link, validation checklist, security impact, trust-boundary, database, blast radius, rollback, and review-track fields.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	Conventional-commits title matches the PR’s memory lifecycle, two-lane retrieval, and after-turn recording changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gemini-code-assist]

Code Review

This pull request implements the host-managed memory lifecycle (mem0 flow) for the Reborn planned loop, introducing proactive two-lane memory retrieval (short-term thread-scoped and long-term user-scoped) and an after-turn interaction recording seam. The feedback highlights a potential latency spike caused by retrying failed memory fetches on every model step, which can be resolved by caching empty results on failure. Additionally, a bug was identified in the interaction recorder where chronological searching might capture a tool call instead of the final assistant reply, and an improvement was suggested to reuse correlation IDs for better distributed tracing.

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]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

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)

tests/support/reborn/harness.rs (1)

861-903: 🎯 Functional Correctness | 🔴 Critical

Missing required field after_turn_memory_writer in this DefaultPlannedRuntimeParts literal.

The struct DefaultPlannedRuntimeParts requires an after_turn_memory_writer field, which is present in other initializations within the codebase but omitted in the literal at tests/support/reborn/harness.rs (lines 861-903). Adding memory_context_service: None without including after_turn_memory_writer results in a compile error (error[E0063]: missing field).

🐛 Proposed fix

             user_profile_source: Arc::new(EmptyUserProfileSource),
             memory_context_service: None,
+            after_turn_memory_writer: None,
             model_policy_guard: None,

🤖 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 `@tests/support/reborn/harness.rs` around lines 861 - 903, The
DefaultPlannedRuntimeParts initializer in build_default_planned_runtime is
missing the required after_turn_memory_writer field, causing the struct literal
to fail to compile. Update this construction to include after_turn_memory_writer
alongside the other runtime dependencies, matching how other
DefaultPlannedRuntimeParts instances are populated in the codebase. Use the
existing build_default_planned_runtime and DefaultPlannedRuntimeParts symbols to
locate the spot and keep the field wiring consistent with the surrounding
memory_context_service setup.

🧹 Nitpick comments (3)

docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md (1)

18-18: 📐 Maintainability & Code Quality | 🔵 Trivial

Optional: specify a language on the fenced code block.

Static analysis flags the flow block as missing a language hint (MD040). Use a fenced language (e.g. ```text) for consistent rendering.

🤖 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/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md` at
line 18, The fenced flow block is missing a language hint and triggers MD040;
update the markdown fence in the spec so the block uses a declared language,
matching the existing fenced block style elsewhere in the document. Locate the
fenced block in the design doc and change it to use a language like text for
consistent rendering and static analysis compliance.

Source: Linters/SAST tools

crates/ironclaw_loop_support/src/lib.rs (1)

1968-1975: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

latest_user_message_text skips memory if the most recent user message is blank, even when an earlier user message has text.

find returns the last User message and the trailing .filter only inspects that one. If the latest user turn is whitespace-only (e.g. an attachment-only message), the query resolves to None and memory is skipped for the run, even though a prior non-blank user message exists. The doc comment ("there is no non-blank user message yet") implies it scans for any non-blank user message.

This degrades gracefully (no memory rather than a failure), so it's low impact, but seeding from the latest non-blank user message would match the documented intent.

♻️ Seed from the latest non-blank user message

 fn latest_user_message_text(messages: &[ContextMessage]) -> Option<String> {
     messages
         .iter()
         .rev()
-        .find(|message| message.kind == MessageKind::User)
-        .map(|message| message.content.clone())
-        .filter(|content| !content.trim().is_empty())
+        .filter(|message| message.kind == MessageKind::User)
+        .map(|message| message.content.clone())
+        .find(|content| !content.trim().is_empty())
 }

🤖 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_loop_support/src/lib.rs` around lines 1968 - 1975, The
latest_user_message_text helper only checks the most recent User message and
then filters out blanks, so a whitespace-only latest turn can suppress memory
even when an earlier non-blank user message exists. Update
latest_user_message_text in lib.rs to search for the latest non-blank User
message directly (i.e., skip blank User messages during the reverse scan) so it
matches the documented intent and returns text from the most recent meaningful
user input.

crates/ironclaw_reborn_composition/src/runtime.rs (1)

3082-3131: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider resolving the document-store provider once and reusing it across the three sinks.

memory_service_resolver.resolve_document_store(Arc::clone(&extension_filesystem), None) is now invoked three times (user_profile_source, memory_context_service, after_turn_memory_writer) with identical inputs, rebuilding the bound provider each time and creating three independent call sites that must stay in lockstep (the inline comments already note this pairing requirement). Resolving once into an Option<Arc<dyn MemoryService>> and cloning it into the three adapters removes the redundant builds and the drift risk.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 3082 - 3131,
Resolve the document-store provider once in runtime.rs instead of calling
memory_service_resolver.resolve_document_store three times for
user_profile_source, memory_context_service, and after_turn_memory_writer. Add a
shared local Option/Arc result near those fields, then clone or map that single
resolved provider into MemoryBackedUserProfileSourceAdapter,
ProductionMemoryPromptContextService::new, and the after_turn_memory_writer sink
so all three stay in lockstep and avoid redundant rebuilds.

🤖 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/service.rs`:
- Around line 396-435: `record_interaction` is appending the same turn history
multiple times because it ignores `run_id` from
`MemoryInvocation`/`TurnRunState`. Update the `record_interaction` flow in
`service.rs` to be idempotent per `run_id`: before calling `self.write` for
`threads/<T>/log.md`, check whether that `run_id` has already been recorded and
skip the append if so, or make the target entry uniquely keyed by `run_id`. Keep
the existing short-term path and `append: true` behavior, but ensure repeated
`execute_claimed_run` / `TurnStatus::Completed` transitions cannot duplicate
context.

---

Outside diff comments:
In `@tests/support/reborn/harness.rs`:
- Around line 861-903: The DefaultPlannedRuntimeParts initializer in
build_default_planned_runtime is missing the required after_turn_memory_writer
field, causing the struct literal to fail to compile. Update this construction
to include after_turn_memory_writer alongside the other runtime dependencies,
matching how other DefaultPlannedRuntimeParts instances are populated in the
codebase. Use the existing build_default_planned_runtime and
DefaultPlannedRuntimeParts symbols to locate the spot and keep the field wiring
consistent with the surrounding memory_context_service setup.

---

Nitpick comments:
In `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 1968-1975: The latest_user_message_text helper only checks the
most recent User message and then filters out blanks, so a whitespace-only
latest turn can suppress memory even when an earlier non-blank user message
exists. Update latest_user_message_text in lib.rs to search for the latest
non-blank User message directly (i.e., skip blank User messages during the
reverse scan) so it matches the documented intent and returns text from the most
recent meaningful user input.

In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3082-3131: Resolve the document-store provider once in runtime.rs
instead of calling memory_service_resolver.resolve_document_store three times
for user_profile_source, memory_context_service, and after_turn_memory_writer.
Add a shared local Option/Arc result near those fields, then clone or map that
single resolved provider into MemoryBackedUserProfileSourceAdapter,
ProductionMemoryPromptContextService::new, and the after_turn_memory_writer sink
so all three stay in lockstep and avoid redundant rebuilds.

In `@docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md`:
- Line 18: The fenced flow block is missing a language hint and triggers MD040;
update the markdown fence in the spec so the block uses a declared language,
matching the existing fenced block style elsewhere in the document. Locate the
fenced block in the design doc and change it to use a language like text for
consistent rendering and static analysis compliance.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e4347cd2-7fb4-4015-b5a6-5afde211af74

📥 Commits

Reviewing files that changed from the base of the PR and between f8377b9 and ff95137.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (24)

• crates/ironclaw_host_runtime/src/memory_context.rs
• crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
• crates/ironclaw_loop_support/src/lib.rs
• crates/ironclaw_memory/Cargo.toml
• crates/ironclaw_memory/src/lib.rs
• crates/ironclaw_memory/src/service.rs
• crates/ironclaw_memory_native/src/lib.rs
• crates/ironclaw_memory_native/src/service.rs
• crates/ironclaw_memory_native/tests/memory_service_facade.rs
• crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs
• crates/ironclaw_product_workflow/tests/support/planned_agent_loop.rs
• crates/ironclaw_reborn/Cargo.toml
• crates/ironclaw_reborn/src/after_turn_memory.rs
• crates/ironclaw_reborn/src/lib.rs
• crates/ironclaw_reborn/src/loop_driver_host.rs
• crates/ironclaw_reborn/src/runtime.rs
• crates/ironclaw_reborn/src/turn_run_executor.rs
• crates/ironclaw_reborn/tests/llm_gateway.rs
• crates/ironclaw_reborn/tests/loop_driver_host.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/tests/product_live_adapters.rs
• crates/ironclaw_turns/tests/agent_loop_host_contract.rs
• docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md
• tests/support/reborn/harness.rs

[BenKurrek]

Thanks @coderabbitai — both findings are addressed in 41a2574b8, alongside an independent adversarial audit and a mem0 data-parity audit run in parallel.

Your findings

• Idempotency on run_id — record_interaction now writes a per-run file threads/<thread_id>/<turn_run_id>.md with append: false (overwrite). A scheduler re-run of a Completed run overwrites idempotently (no duplicate snippets), and growth is bounded to one file per run (no unbounded log.md).
• Missing after_turn_memory_writer (E0063) — added to tests/support/reborn/harness.rs. The root crate's reborn tests now compile (verified locally with cargo test -p ironclaw --no-run --test reborn_agent_scope_isolation_parity).

Also fixed this pass (audit + mem0 data parity)

• Multi-step runs recorded the first assistant, not the final answer → build_exchange → build_transcript now captures the full ordered run transcript (every user/assistant/tool message; every finalized assistant including the final one).
• run_id-vs-session conflation → renamed MemoryServiceRecordRequest.run_id → turn_run_id (per-turn provenance); mem0's session id maps to scope.thread_id (documented on the contract so a provider can't mis-map and silently miss short-term recall).
• Added MemoryInteractionMessage.name (per-message actor → mem0 actor_id) + provenance metadata; fixed a per-run memory cache that could freeze to empty when the first prompt build had no user message; added arch-exempt: optional_arc annotations; doc corrections (long-term full-tuple scope, the threads/ advisory reservation, the v1 no-mid-run-invalidation note).

Requesting a full re-review.

[BenKurrek]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

@BenKurrek Acknowledged. I’ll re-run a full review on #5327 with the updated contract in mind — especially the per-run overwrite/idempotency path, thread-vs-user scope separation, and the turn_run_id provenance rename.

---

✏️ Learnings added

Learnt from: BenKurrek
URL: https://github.com/nearai/ironclaw/pull/5327

Timestamp: 2026-06-26T14:17:32.808Z
Learning: In this repository's mem0 memory recording flow, `MemoryServiceRecordRequest.turn_run_id` is the per-turn provenance identifier; mem0 session identity maps to `MemoryInvocation.scope.thread_id`, not to `turn_run_id`.

---

Learnt from: BenKurrek
URL: https://github.com/nearai/ironclaw/pull/5327

Timestamp: 2026-06-26T14:17:32.809Z
Learning: For the native memory provider's after-turn recording path, `record_interaction` writes one file per completed run at `threads/<thread_id>/<turn_run_id>.md` with overwrite semantics (`append: false`) so scheduler replays of a completed run remain idempotent and do not append to an unbounded shared `log.md`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 6

🤖 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 398-416: The budget-path test in memory_prompt_context.rs can
still pass with an empty snippets result, so add an explicit non-empty assertion
on the output from load_memory_snippets in the same test before checking
total_bytes and short_term_refs. Use the existing snippets variable and assert
that at least one snippet is returned, then keep the current budget and
short-term precedence checks so a regression that drops all candidates is
caught.

In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 366-381: The thread-aware filtering in service.rs is happening
after backend.search() has already been capped by request.max_snippets, so
short-term calls can lose thread-matching results when global hits rank higher.
Fix the lane selection logic in the search path around
invocation.scope.thread_id by applying the thread constraint before truncation,
either by pushing the prefix/exclusion into backend.search() or by over-fetching
and only enforcing request.max_snippets after the retain filter.
- Around line 661-686: The reserved `threads/` namespace is only being treated
as retrieval-only right now, but the write path still allows tool-authored
documents to be persisted there. Update the write handling in `service.rs` to
explicitly reject any relative path matching `THREAD_MEMORY_ROOT` before saving,
using the existing `is_thread_scoped_path`/`THREAD_MEMORY_ROOT` checks, and
return a clear failure instead of silently accepting the write. Make sure the
rejection is applied at the point where tool-originated writes are validated so
`threads/...` cannot be stored at all.

In `@crates/ironclaw_memory_native/tests/memory_service_facade.rs`:
- Around line 689-700: The test for record_interaction is not isolating the
threadless path because turn_run_id is also missing, so it can pass via a
generic no-op instead of exercising the intended branch. Update the
MemoryServiceRecordRequest in memory_service_facade::record_interaction to
supply a real turn_run_id while keeping thread_id absent, so the test
specifically verifies the “no thread to record under” behavior.

In `@crates/ironclaw_reborn/src/after_turn_memory.rs`:
- Around line 180-187: Remove the trimming step in the message-to-interaction
conversion so the original transcript text is preserved for record_interaction;
in after_turn_memory.rs, update the logic around MemoryInteractionMessage
construction to still filter out empty/blank content but pass the
message.content through unchanged instead of using map(str::trim), keeping the
full ordered transcript intact.

In `@docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md`:
- Around line 43-50: The spec still contains outdated memory-contract text that
conflicts with the current scope-based lane selection and per-run thread log
design. Rewrite the earlier sections to match the final contract everywhere in
this document: remove the superseded `thread_id` metadata/filtering and mid-run
invalidation language, replace `run_id` and `threads/<thread_id>/log.md`
references with the current per-run `threads/<thread_id>/<turn_run_id>.md`
shape, and align the `short_term` and host `add` descriptions with the shipped
behavior. Update the affected spec sections consistently so there is one
authoritative design narrative rather than layered corrections.

🪄 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: e633ff8e-869d-409a-86c6-a35c75b1af87

📥 Commits

Reviewing files that changed from the base of the PR and between f8377b9 and 41a2574.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock

📒 Files selected for processing (24)

• crates/ironclaw_host_runtime/src/memory_context.rs
• crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
• crates/ironclaw_loop_support/src/lib.rs
• crates/ironclaw_memory/Cargo.toml
• crates/ironclaw_memory/src/lib.rs
• crates/ironclaw_memory/src/service.rs
• crates/ironclaw_memory_native/src/lib.rs
• crates/ironclaw_memory_native/src/service.rs
• crates/ironclaw_memory_native/tests/memory_service_facade.rs
• crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs
• crates/ironclaw_product_workflow/tests/support/planned_agent_loop.rs
• crates/ironclaw_reborn/Cargo.toml
• crates/ironclaw_reborn/src/after_turn_memory.rs
• crates/ironclaw_reborn/src/lib.rs
• crates/ironclaw_reborn/src/loop_driver_host.rs
• crates/ironclaw_reborn/src/runtime.rs
• crates/ironclaw_reborn/src/turn_run_executor.rs
• crates/ironclaw_reborn/tests/llm_gateway.rs
• crates/ironclaw_reborn/tests/loop_driver_host.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/tests/product_live_adapters.rs
• crates/ironclaw_turns/tests/agent_loop_host_contract.rs
• docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md
• tests/support/reborn/harness.rs

[coderabbitai]

Actionable comments posted: 3

🤖 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_reborn/src/runtime.rs`:
- Around line 223-230: Soften the contract comment in runtime.rs around the
after-turn memory writer to match what record_completed_run() actually does.
Update the description near the Completed run handling and record_interaction
path so it says the executor forwards the full run transcript and only skips
runs without user/assistant content, rather than guaranteeing a strict [user,
assistant] pair. Keep the note about None degrading to no after-turn recording.

In `@crates/ironclaw_reborn/tests/loop_driver_host.rs`:
- Around line 1605-1727: Cover the real runtime wiring by adding a caller-level
test that goes through build_default_planned_runtime and verifies
after_turn_memory_writer is plumbed into the executor; the current
turn_runner_worker_records_after_turn_memory_on_completed_run test only
exercises with_after_turn_memory_recorder directly. Create a test that
constructs DefaultPlannedRuntimeParts with after_turn_memory_writer: Some(...)
and asserts the per-run thread doc is written after a completed run, so the
actual call site in build_default_planned_runtime_inner cannot regress
unnoticed.

In `@crates/ironclaw_turns/tests/agent_loop_host_contract.rs`:
- Around line 581-629: The memory section currently only checks that a
`msg:memory.*` message exists, but it does not verify that short-term snippets
keep priority over long-term ones at the render boundary. Update
`instruction_bundle_renders_memory_section_from_memory_snippets` to assert the
ordering of the rendered memory messages, and fix
`InstructionBundleBuilder::build` so it preserves the incoming
`LoopContextBundle.memory_snippets` insertion order instead of re-sorting with
`compare_snippet_refs`. Focus on the memory materialization path and the
model-visible `messages`/`materialized_messages` output.

🪄 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: 11e4279b-3044-44ba-a8cc-f2e52a6cc09f

📥 Commits

Reviewing files that changed from the base of the PR and between f8377b9 and 41a2574.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock

📒 Files selected for processing (24)

• crates/ironclaw_host_runtime/src/memory_context.rs
• crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
• crates/ironclaw_loop_support/src/lib.rs
• crates/ironclaw_memory/Cargo.toml
• crates/ironclaw_memory/src/lib.rs
• crates/ironclaw_memory/src/service.rs
• crates/ironclaw_memory_native/src/lib.rs
• crates/ironclaw_memory_native/src/service.rs
• crates/ironclaw_memory_native/tests/memory_service_facade.rs
• crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs
• crates/ironclaw_product_workflow/tests/support/planned_agent_loop.rs
• crates/ironclaw_reborn/Cargo.toml
• crates/ironclaw_reborn/src/after_turn_memory.rs
• crates/ironclaw_reborn/src/lib.rs
• crates/ironclaw_reborn/src/loop_driver_host.rs
• crates/ironclaw_reborn/src/runtime.rs
• crates/ironclaw_reborn/src/turn_run_executor.rs
• crates/ironclaw_reborn/tests/llm_gateway.rs
• crates/ironclaw_reborn/tests/loop_driver_host.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/tests/product_live_adapters.rs
• crates/ironclaw_turns/tests/agent_loop_host_contract.rs
• docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md
• tests/support/reborn/harness.rs

[BenKurrek]

Re-review round addressed — dfce4cadc

All 10 open review threads (7 CodeRabbit + 3 Gemini) are fixed and resolved. Behavioral changes are TDD'd (red test shown to fail first, then made green).

CodeRabbit

• Short-term lane starvation (service.rs retrieve_context) — over-fetch (max_snippets*8, min 64) before the scope+lane retains, truncate to max_snippets only after the lane filter. Test: native_short_term_retrieval_over_fetches_before_thread_lane_filter.
• Reserved threads/ writes (service.rs write) — now rejected (fail loud); record_interaction writes there via a private write_reserved_document bypass. Test: native_write_rejects_reserved_thread_namespace. (Un-defers audit L1.)
• No transcript trim (after_turn_memory.rs build_transcript) — filter blank-only rows, pass content verbatim. Test: build_transcript_preserves_content_verbatim_and_filters_blanks.
• Render-boundary lane priority (instruction_bundle.rs) — dropped the by-ref re-sort so the host's short-term-first order survives. Test: instruction_bundle_renders_memory_section_from_memory_snippets (+ repurposed the old model_content-tiebreak test).
• Caller-level wiring test — build_default_planned_runtime_wires_after_turn_memory_writer drives the real composition factory and asserts the per-run doc is written.
• Test rigor + comment — threadless record_interaction test now sets a real turn_run_id; softened the after-turn writer contract comments to match full-transcript behavior.

Gemini

• get_or_try_init retry-storm (loop_support load_memory_snippets_once) — switched to get_or_init caching empty on failure = true fetch-once-per-run (M1 early-return preserved). Behavior change — a transient first-fetch failure now degrades memory to empty for the rest of that run rather than retrying each model step.
• Final assistant via .find() — already addressed on this branch: build_transcript records the full ordered transcript (every finalized assistant + the final answer), so it no longer depends on .find(). Resolved as addressed.
• Shared correlation id — both retrieval lanes now share one correlation_id.

Also: strengthened the aggregate-budget test (non-empty assertion so the all-short-term check isn't vacuous); aligned the design doc with shipped behavior (no mid-run invalidation; full transcript; threads/ enforced).

cargo fmt + clippy clean (zero warnings); per-crate tests green across ironclaw_memory_native, ironclaw_host_runtime, ironclaw_turns, ironclaw_loop_support, ironclaw_reborn.

[BenKurrek]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (2)

crates/ironclaw_reborn_composition/src/runtime.rs (1)

3117-3122: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Soften the after-turn recording contract here too.

Line 3120 still says the executor records a strict [user, assistant] exchange, but AfterTurnMemoryRecorder::record_completed_run() forwards the full transcript and only skips runs with no user/assistant content. This recreates the same cross-layer doc drift already fixed in crates/ironclaw_reborn/src/runtime.rs.

Suggested diff

-        // The executor records each Completed run's `[user, assistant]` exchange
-        // through `record_interaction`. `None` degrades to no after-turn recording,
+        // The executor forwards each Completed run's full transcript to
+        // `record_interaction`, skipping only runs with no user/assistant content.
+        // `None` degrades to no after-turn recording,

As per coding guidelines, "Comments that promise guarantees across layers must either be enforced by code/tests or softened to describe 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_reborn_composition/src/runtime.rs` around lines 3117 - 3122,
The comment in runtime.rs overstates the after-turn recording contract by saying
the executor records a strict [user, assistant] exchange, but
AfterTurnMemoryRecorder::record_completed_run() actually forwards the full
transcript and only skips runs with no user/assistant content. Soften the
wording in the nearby comment to describe the intent and fallback behavior
instead of guaranteeing a strict two-message shape, keeping it aligned with the
behavior already reflected in ironclaw_reborn/src/runtime.rs.

Source: Coding guidelines

docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md (1)

19-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Collapse this spec onto the shipped memory contract.

These sections still describe the superseded design: long_term before short_term, thread_id metadata/filter-based retrieval, after-turn writes feeding both lanes, and threads/<thread_id>/log.md. The implemented contract in this PR is short-term-first ordering, scope-based lane selection, and per-run threads/<thread_id>/<turn_run_id>.md that feeds only the short-term lane. As per coding guidelines, **/*.md: "After a refactor that relocates or renames code, grep for .md and CLAUDE.md references to the moved paths and update them in the same PR".

Also applies to: 43-51, 93-96, 145-149

🤖 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/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md`
around lines 19 - 29, Update this memory lifecycle spec to match the shipped
contract by removing the superseded long_term-first, thread_id filter/metadata
retrieval, and after-turn dual-lane write descriptions. In the sections around
on_run_start, before_model_call, after_each_turn, and on_run_end, describe
short-term-first ordering, scope-based lane selection, and per-run
threads/<thread_id>/<turn_run_id>.md files that feed only the short-term lane.
Also grep related markdown references such as the threads/<thread_id>/log.md
path and replace any stale path or lifecycle wording so the spec reflects the
current implementation.

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_loop_support/src/lib.rs`:
- Around line 1979-1985: The helper latest_user_message_text currently returns
None when the newest user message is blank, even if an earlier MessageKind::User
message has content; update it to scan backward for the latest non-blank user
message instead of filtering after selecting the first user row. Keep the logic
localized in latest_user_message_text and ensure load_loop_context still uses it
to decide whether to call build_memory_prompt_context_request(). Add a
regression test through load_loop_context that covers a trailing blank user
message followed by an earlier non-blank user turn, and verify the memory fetch
path still runs.

In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 460-477: The write_reserved_document helper currently relies on
callers to honor the reserved threads/ contract, but it never enforces it
itself. Add an explicit target check in write_reserved_document (and/or a
focused test) to reject any non-threads/ path before
resolve_target_path/document_path is used, so the bypass guarantee matches the
MemoryService::record_interaction-only intent.

In `@crates/ironclaw_reborn/src/after_turn_memory.rs`:
- Around line 80-91: The `after_turn_memory` fallback paths are swallowing
thread-history read and memory-write errors without the required explicit
annotation. Add inline `// silent-ok: <reason>` comments at the
`list_thread_history` error branch and the memory-write failure branch to
document the intentional boundary fallback. Use the existing `after-turn memory`
handling in `after_turn_memory.rs` as the place to annotate these swallowed
errors so audits can distinguish deliberate degradation from accidental error
loss.

In `@crates/ironclaw_reborn/src/turn_run_executor.rs`:
- Around line 295-298: The post-completion call in
turn_run_executor::TurnRunExecutor::execute is awaiting
recorder.record_completed_run inline, which can block the scheduler worker.
Update the Completed branch to offload this best-effort side effect via a
bounded background task/worker or wrap the await in a short timeout, while
keeping the existing state check and after_turn_memory_recorder lookup intact.

In `@crates/ironclaw_reborn/tests/loop_driver_host.rs`:
- Around line 1674-1716: The test is shutting down the worker too early, which
can cancel the after-turn memory recorder before the thread log is written. In
the loop_driver_host test, keep polling MemoryServiceReadRequest via
memory_writer until the log at the computed path is read and asserted, and only
call scheduler_handle.shutdown() after those memory assertions complete. Use the
existing wait_for_run_status, memory_writer.read, and scheduler_handle symbols
to keep the sequencing stable.

---

Duplicate comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3117-3122: The comment in runtime.rs overstates the after-turn
recording contract by saying the executor records a strict [user, assistant]
exchange, but AfterTurnMemoryRecorder::record_completed_run() actually forwards
the full transcript and only skips runs with no user/assistant content. Soften
the wording in the nearby comment to describe the intent and fallback behavior
instead of guaranteeing a strict two-message shape, keeping it aligned with the
behavior already reflected in ironclaw_reborn/src/runtime.rs.

In `@docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md`:
- Around line 19-29: Update this memory lifecycle spec to match the shipped
contract by removing the superseded long_term-first, thread_id filter/metadata
retrieval, and after-turn dual-lane write descriptions. In the sections around
on_run_start, before_model_call, after_each_turn, and on_run_end, describe
short-term-first ordering, scope-based lane selection, and per-run
threads/<thread_id>/<turn_run_id>.md files that feed only the short-term lane.
Also grep related markdown references such as the threads/<thread_id>/log.md
path and replace any stale path or lifecycle wording so the spec reflects the
current implementation.

🪄 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: 293c6347-7ae7-490d-8961-628757daeee5

📥 Commits

Reviewing files that changed from the base of the PR and between f8377b9 and dfce4ca.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock

📒 Files selected for processing (25)

• crates/ironclaw_host_runtime/src/memory_context.rs
• crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
• crates/ironclaw_loop_support/src/lib.rs
• crates/ironclaw_memory/Cargo.toml
• crates/ironclaw_memory/src/lib.rs
• crates/ironclaw_memory/src/service.rs
• crates/ironclaw_memory_native/src/lib.rs
• crates/ironclaw_memory_native/src/service.rs
• crates/ironclaw_memory_native/tests/memory_service_facade.rs
• crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs
• crates/ironclaw_product_workflow/tests/support/planned_agent_loop.rs
• crates/ironclaw_reborn/Cargo.toml
• crates/ironclaw_reborn/src/after_turn_memory.rs
• crates/ironclaw_reborn/src/lib.rs
• crates/ironclaw_reborn/src/loop_driver_host.rs
• crates/ironclaw_reborn/src/runtime.rs
• crates/ironclaw_reborn/src/turn_run_executor.rs
• crates/ironclaw_reborn/tests/llm_gateway.rs
• crates/ironclaw_reborn/tests/loop_driver_host.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/tests/product_live_adapters.rs
• crates/ironclaw_turns/src/run_profile/instruction_bundle.rs
• crates/ironclaw_turns/tests/agent_loop_host_contract.rs
• docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md
• tests/support/reborn/harness.rs

[BenKurrek]

Round 3 addressed + base merged — fb7447762

Merged reborn/memory-lift-followups (#5205) into the branch — resolved the single crates/ironclaw_memory/src/service.rs conflict by combining both independently-added mod tests blocks. Then fixed all 5 new findings + the duplicate from the last full review:

• Latest non-blank user message (loop_support) — latest_user_message_text uses find_map to skip a blank trailing user turn, so proactive memory isn't dropped when an earlier turn has content (+ unit test).
• Reserved-write guard (memory_native) — write_reserved_document now rejects any non-threads/ resolved path, enforcing the bypass contract so the public write guard can't be circumvented.
• silent-ok annotations (after_turn_memory) — both post-terminal fallbacks (history read + provider write) annotated per the fail-loud convention.
• Bounded recorder (turn_run_executor) — the inline after-turn await is wrapped in a 30s timeout, so a slow/hung provider can't occupy the scheduler worker.
• Test race (loop_driver_host) — the sibling test now shuts the scheduler down after the memory read/assertions.
• Duplicate (reborn_composition) — softened the [user, assistant] contract comment to match full-transcript behavior (same fix as runtime.rs).

All round-3 threads resolved. fmt + clippy clean; tests green (loop_support 336, memory_native 20, reborn 258 + loop_driver_host 109); full workspace compiles post-merge.

[BenKurrek]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

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_reborn_composition/src/runtime.rs (1)

3097-3147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

resolve_document_store is now resolved three times over the same filesystem.

user_profile_source (Line 3097), memory_context_service (Line 3120), and after_turn_memory_writer (Line 3139) each independently call local_runtime.memory_service_resolver.resolve_document_store(Arc::clone(&extension_filesystem), None), building three separate provider instances for one runtime. Resolve once into a local Option<Arc<dyn MemoryService>> and map it into the three shapes (profile adapter / prompt-context wrap / raw) to keep one construction point — which the surrounding comments already claim is the 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_reborn_composition/src/runtime.rs` around lines 3097 - 3147,
The same document-store provider is being resolved three times from
local_runtime, which creates duplicate instances instead of one shared runtime
binding. Refactor the runtime setup in runtime.rs to resolve
local_runtime.memory_service_resolver.resolve_document_store(...) once into a
local Option and reuse that value for user_profile_source,
memory_context_service, and after_turn_memory_writer, mapping it into the
appropriate adapter/wrapper/raw forms via MemoryBackedUserProfileSource::new,
ProductionMemoryPromptContextService::new, and the direct Arc value.

♻️ Duplicate comments (1)

docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md (1)

19-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Spec still layers corrections instead of one authoritative contract.

The flow pseudocode (filter={thread_id}, line 21), Surface A (tag writes with thread_id on DocumentMetadata + thread_id filter on search, lines 43-46), Q3 (record_interaction(messages, run_id, ...), line 79), and the Phase-2 log (threads/<thread_id>/log.md, line 148) all contradict the shipped scope-based lane selection and per-run threads/<thread_id>/<turn_run_id>.md. The progress log corrects these below rather than fixing them in place, so the document still self-contradicts. As per coding guidelines (**/*.md: "grep for .md references to the moved paths and update them in the same PR").

Also applies to: 43-50, 146-149

🤖 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/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md`
around lines 19 - 29, The spec has conflicting memory-lifecycle contracts across
the pseudocode, Surface A, Q3, and the Phase-2 log, so update all affected
references to one authoritative model instead of layering corrections. Align the
flow in the main lifecycle section, the `record_interaction`/Q3 text, and the
log/path examples so they all use the same scope-based lane selection and
per-run `threads/<thread_id>/<turn_run_id>.md` structure, and remove any stale
`thread_id`-filter or `threads/<thread_id>/log.md` wording that contradicts it.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 146-164: The two independent `retrieve_lane` calls in
`MemoryContext::retrieve_context` are currently awaited sequentially, which
unnecessarily adds latency on the run-start prompt path. Update this logic to
fetch the short-term and long-term lanes concurrently using `tokio::join!`,
while preserving the existing short-term-first ordering when combining results;
keep the shared `context_profile_id` and `request.max_snippets` handling intact
and use the existing `retrieve_lane` calls as the key symbols to locate the
change.

---

Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3097-3147: The same document-store provider is being resolved
three times from local_runtime, which creates duplicate instances instead of one
shared runtime binding. Refactor the runtime setup in runtime.rs to resolve
local_runtime.memory_service_resolver.resolve_document_store(...) once into a
local Option and reuse that value for user_profile_source,
memory_context_service, and after_turn_memory_writer, mapping it into the
appropriate adapter/wrapper/raw forms via MemoryBackedUserProfileSource::new,
ProductionMemoryPromptContextService::new, and the direct Arc value.

---

Duplicate comments:
In `@docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md`:
- Around line 19-29: The spec has conflicting memory-lifecycle contracts across
the pseudocode, Surface A, Q3, and the Phase-2 log, so update all affected
references to one authoritative model instead of layering corrections. Align the
flow in the main lifecycle section, the `record_interaction`/Q3 text, and the
log/path examples so they all use the same scope-based lane selection and
per-run `threads/<thread_id>/<turn_run_id>.md` structure, and remove any stale
`thread_id`-filter or `threads/<thread_id>/log.md` wording that contradicts it.

🪄 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: 92d30a1a-719b-409f-8a40-68e50330ba1f

📥 Commits

Reviewing files that changed from the base of the PR and between 502f676 and fb74477.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock

📒 Files selected for processing (25)

• crates/ironclaw_host_runtime/src/memory_context.rs
• crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
• crates/ironclaw_loop_support/src/lib.rs
• crates/ironclaw_memory/Cargo.toml
• crates/ironclaw_memory/src/lib.rs
• crates/ironclaw_memory/src/service.rs
• crates/ironclaw_memory_native/src/lib.rs
• crates/ironclaw_memory_native/src/service.rs
• crates/ironclaw_memory_native/tests/memory_service_facade.rs
• crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs
• crates/ironclaw_product_workflow/tests/support/planned_agent_loop.rs
• crates/ironclaw_reborn/Cargo.toml
• crates/ironclaw_reborn/src/after_turn_memory.rs
• crates/ironclaw_reborn/src/lib.rs
• crates/ironclaw_reborn/src/loop_driver_host.rs
• crates/ironclaw_reborn/src/runtime.rs
• crates/ironclaw_reborn/src/turn_run_executor.rs
• crates/ironclaw_reborn/tests/llm_gateway.rs
• crates/ironclaw_reborn/tests/loop_driver_host.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/tests/product_live_adapters.rs
• crates/ironclaw_turns/src/run_profile/instruction_bundle.rs
• crates/ironclaw_turns/tests/agent_loop_host_contract.rs
• docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md
• tests/support/reborn/harness.rs

[ironloopai]

❌ IronLoop Review: reviewer

Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: 404760cf45efe5795b6cf56b5a0d948a1c79b2ef

Run details

Status: Current
Needs human: no
Needs validation: no

**Inline candidates:** 1

Summary

Found a runtime behavior regression in the new two-lane memory prompt assembly: the host admits long-term snippets before short-term snippets, so the active thread memory can be starved under the shared snippet/count budget despite the render layer preserving host order for short-term priority.

Findings

1. ❌ [MEDIUM] Short-term memory can be starved by long-term results

Location: crates/ironclaw_host_runtime/src/memory_context.rs:91
The combined admission loop consumes long_term before short_term and stops at request.max_snippets or the aggregate byte cap. If long-term retrieval returns enough admissible snippets, the active thread's short-term memory is never surfaced. This contradicts the render-side contract added in instruction_bundle.rs that memory arrives in host order with short-term before long-term so the active conversation keeps priority under the shared budget. Chain short_term before long_term here, and update the host-runtime tests that currently assert long-term wins under budget pressure.

Developer follow-up

After fixing this feedback:

1. Push the fix to this PR branch.
2. Re-run this reviewer with @ironloop review --agent reviewer if you only changed this reviewer's findings.
3. Re-run all reviewers with @ironloop review when the fix may affect multiple areas.
4. Use @ironloop status to check queued/running/completed/stale/stalled state while reviewers run.

[ironloopai]

This admits long-term snippets before short-term snippets, then stops at the shared count/byte budget. When long-term returns enough hits, the active thread lane is completely starved, even though the render layer now preserves host order specifically so short-term memory keeps priority under budget pressure. Please chain short_term before long_term and update the budget/order tests accordingly.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (2)

crates/ironclaw_reborn_composition/src/runtime.rs (1)

3458-3466: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Still resolving the document-store provider three times — same concern flagged in a prior review round.

memory_service_resolver.resolve_document_store(...) is called separately for user_profile_source, memory_context_service, and after_turn_memory_writer, with identical arguments each time. The in-code comments explicitly promise these three consumers share "the SAME document-store provider" / "the SAME memory_service_resolver" — but the code doesn't actually guarantee a single instance. If resolution isn't a cheap, referentially-stable lookup, prompt reads and after-turn writes can drift onto different provider instances, silently breaking the shared-cache/consistency assumption noted elsewhere in this PR ("fixed a cache issue that could freeze memory to empty").

♻️ Resolve once, fan out from the shared handle

+        let resolved_memory_provider = local_runtime.and_then(|local_runtime| {
+            local_runtime.memory_service_resolver.resolve_document_store(
+                Arc::clone(&local_runtime.extension_filesystem)
+                    as Arc<dyn ironclaw_filesystem::RootFilesystem>,
+                None,
+            )
+        });
+
         user_profile_source: match local_runtime.and_then(|local_runtime| {
-            local_runtime
-                .memory_service_resolver
-                .resolve_document_store(
-                    Arc::clone(&local_runtime.extension_filesystem)
-                        as Arc<dyn ironclaw_filesystem::RootFilesystem>,
-                    None,
-                )
-                .map(MemoryBackedUserProfileSource::new)
+            resolved_memory_provider
+                .clone()
+                .map(MemoryBackedUserProfileSource::new)
         }) {
             Some(source) => Arc::new(MemoryBackedUserProfileSourceAdapter(source))
                 as Arc<dyn HostUserProfileSource>,
             None => Arc::new(EmptyUserProfileSource) as Arc<dyn HostUserProfileSource>,
         },
-        memory_context_service: local_runtime
-            .and_then(|local_runtime| {
-                local_runtime
-                    .memory_service_resolver
-                    .resolve_document_store(
-                        Arc::clone(&local_runtime.extension_filesystem)
-                            as Arc<dyn ironclaw_filesystem::RootFilesystem>,
-                        None,
-                    )
-                    .map(ProductionMemoryPromptContextService::new)
-            })
+        memory_context_service: resolved_memory_provider
+            .clone()
+            .map(ProductionMemoryPromptContextService::new)
             .map(|service| Arc::new(service) as Arc<dyn MemoryPromptContextService>),
-        after_turn_memory_writer: local_runtime.and_then(|local_runtime| {
-            local_runtime
-                .memory_service_resolver
-                .resolve_document_store(
-                    Arc::clone(&local_runtime.extension_filesystem)
-                        as Arc<dyn ironclaw_filesystem::RootFilesystem>,
-                    None,
-                )
-        }),
+        after_turn_memory_writer: resolved_memory_provider,

Also applies to: 3481-3492, 3500-3508

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 3458 - 3466,
Resolve the document-store provider once in runtime.rs and reuse the same handle
for user_profile_source, memory_context_service, and after_turn_memory_writer
instead of calling memory_service_resolver.resolve_document_store(...)
separately for each. Update the shared setup around local_runtime so the result
of resolve_document_store is stored in a single variable and fanned out to
MemoryBackedUserProfileSource::new, MemoryBackedMemoryContextService::new, and
the after-turn writer path, preserving the “same document-store provider”
assumption across all three consumers.

crates/ironclaw_memory_native/src/service.rs (1)

137-139: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Still unresolved: threads/ prefix checks remain case-sensitive.

is_thread_scoped_path/thread_memory_prefix (and every call site: the write reservation guard, and both branches of the lane split in retrieve_context) still use plain starts_with. A previously-flagged concern on this exact code (case-insensitive threads/ comparison) doesn't have a corresponding fix confirmation in this PR's history, unlike the sibling fixes for idempotency, over-fetch ordering, and reserved-write enforcement. On macOS/Windows, Threads/foo.md aliases the reserved threads/foo.md path on disk but slips past both the public write guard and the lane retains — the same "silent retrieval black hole" this PR otherwise closes.

🛡️ Proposed fix

 fn thread_memory_prefix(thread_id: &ThreadId) -> String {
-    format!("{THREAD_MEMORY_ROOT}{}/", thread_id.as_str())
+    format!("{THREAD_MEMORY_ROOT}{}/", thread_id.as_str()).to_ascii_lowercase()
 }

 fn is_thread_scoped_path(relative_path: &str) -> bool {
-    relative_path.starts_with(THREAD_MEMORY_ROOT)
+    relative_path.to_ascii_lowercase().starts_with(THREAD_MEMORY_ROOT)
 }

And normalize the lane-retain comparisons (result.path.relative_path().starts_with(&prefix) at line 392, and the same pattern via is_thread_scoped_path at line 398) the same way.

Also applies to: 385-405, 721-729

🤖 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 137 - 139,
`is_thread_scoped_path` and `thread_memory_prefix` still do case-sensitive
`starts_with` checks, so reserved `threads/` paths can slip through on
case-insensitive filesystems. Update the path-prefix comparisons used by `write`
and both `retrieve_context` lane branches to compare the normalized prefix
case-insensitively, and make the retain checks on `result.path.relative_path()`
use the same normalized logic so `Threads/...` is treated the same as
`threads/...`.

🤖 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_loop_support/src/lib.rs`:
- Around line 219-230: The new memory-context wiring is being added inline in
lib.rs, which should stay a thin facade rather than hosting a whole new context
source. Move the memory-related pieces — memory_context_service,
with_memory_context_service, load_memory_snippets_once,
build_memory_prompt_context_request, and latest_user_message_text — into a
dedicated module such as src/memory.rs, then re-export the public entry points
from lib.rs. Keep lib.rs focused on adapter/context-source exposure and preserve
the existing memory_snippets_cache behavior and API surface through the new
module.

In `@crates/ironclaw_reborn/tests/loop_driver_host.rs`:
- Around line 1697-1727: The after-turn memory document polling logic is
duplicated in the text-host tests, including the repeated
MemoryInvocation/ResourceScope setup and the retry loop that reads
threads/<thread_id>/<run_id>.md. Extract this into a shared async helper near
the existing test utilities in loop_driver_host.rs, using the same
MemoryInvocation and read loop behavior, then update both call sites to use the
helper so the timeout/backoff policy stays consistent.

---

Duplicate comments:
In `@crates/ironclaw_memory_native/src/service.rs`:
- Around line 137-139: `is_thread_scoped_path` and `thread_memory_prefix` still
do case-sensitive `starts_with` checks, so reserved `threads/` paths can slip
through on case-insensitive filesystems. Update the path-prefix comparisons used
by `write` and both `retrieve_context` lane branches to compare the normalized
prefix case-insensitively, and make the retain checks on
`result.path.relative_path()` use the same normalized logic so `Threads/...` is
treated the same as `threads/...`.

In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3458-3466: Resolve the document-store provider once in runtime.rs
and reuse the same handle for user_profile_source, memory_context_service, and
after_turn_memory_writer instead of calling
memory_service_resolver.resolve_document_store(...) separately for each. Update
the shared setup around local_runtime so the result of resolve_document_store is
stored in a single variable and fanned out to
MemoryBackedUserProfileSource::new, MemoryBackedMemoryContextService::new, and
the after-turn writer path, preserving the “same document-store provider”
assumption across all three consumers.

🪄 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: fa9c0541-ad1e-4c8e-9734-a712feaa404a

📥 Commits

Reviewing files that changed from the base of the PR and between a0310d6 and 404760c.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock

📒 Files selected for processing (29)

• crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
• crates/ironclaw_host_runtime/src/memory_context.rs
• crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
• crates/ironclaw_loop_support/src/lib.rs
• crates/ironclaw_memory/Cargo.toml
• crates/ironclaw_memory/src/lib.rs
• crates/ironclaw_memory/src/service.rs
• crates/ironclaw_memory_native/src/lib.rs
• crates/ironclaw_memory_native/src/service.rs
• crates/ironclaw_memory_native/tests/memory_service_facade.rs
• crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs
• crates/ironclaw_product_workflow/tests/support/planned_agent_loop.rs
• crates/ironclaw_reborn/Cargo.toml
• crates/ironclaw_reborn/src/after_turn_memory.rs
• crates/ironclaw_reborn/src/lib.rs
• crates/ironclaw_reborn/src/loop_driver_host.rs
• crates/ironclaw_reborn/src/runtime.rs
• crates/ironclaw_reborn/src/turn_run_executor.rs
• crates/ironclaw_reborn/tests/llm_gateway.rs
• crates/ironclaw_reborn/tests/loop_driver_host.rs
• crates/ironclaw_reborn_composition/src/runtime.rs
• crates/ironclaw_reborn_composition/tests/product_live_adapters.rs
• crates/ironclaw_turns/src/run_profile/instruction_bundle.rs
• crates/ironclaw_turns/tests/agent_loop_host_contract.rs
• docs/superpowers/specs/2026-06-25-reborn-memory-host-lifecycle-design.md
• tests/integration/support/group.rs
• tests/integration/support/planned_runtime_parts_shape.rs
• tests/integration/wiring_parity.rs
• tests/support/reborn_parity_qa/binary_e2e.rs

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Extract the new memory-context wiring into its own module.

This block adds a new context source (memory_context_service field, with_memory_context_service, load_memory_snippets_once, build_memory_prompt_context_request, latest_user_message_text) directly into lib.rs, which is already a large, heavily-touched file. As per coding guidelines, crates/ironclaw_loop_support/src/**/*.rs should "Add one file per host adapter or context source," and separately, touched files in the 1,500–3,000 line band "should get shorter unless a larger feature justifies growth." Moving this to e.g. src/memory.rs and re-exporting the public builder from lib.rs keeps the facade role of lib.rs intact ("Expose loop support adapters... from src/lib.rs") without folding a whole new context source inline.

Also applies to: 308-319, 419-525, 1996-2009

🤖 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_loop_support/src/lib.rs` around lines 219 - 230, The new
memory-context wiring is being added inline in lib.rs, which should stay a thin
facade rather than hosting a whole new context source. Move the memory-related
pieces — memory_context_service, with_memory_context_service,
load_memory_snippets_once, build_memory_prompt_context_request, and
latest_user_message_text — into a dedicated module such as src/memory.rs, then
re-export the public entry points from lib.rs. Keep lib.rs focused on
adapter/context-source exposure and preserve the existing memory_snippets_cache
behavior and API surface through the new module.

Source: Coding guidelines

[ironloopai]

🗂️ Archived IronLoop Review: reviewer

This result is from an older PR head and is no longer the active review.

Field	Value
Status	Superseded
Verdict	✅ Approved
Findings	0 blocking / 0 notes
Reviewed head	8522b3512d34

Archived summary

No concrete blocking issues found in the memory context and after-turn recording changes. The PR adds scoped long/short-term memory retrieval, provider-neutral snippet sanitization, native thread-scoped recording, and focused regression coverage.

[BenKurrek]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[BenKurrek]

/canary

[github-actions]

Started Reborn WebUI v2 live canary for reborn/memory-lifecycle at 8522b3512d with cases all: https://github.com/nearai/ironclaw/actions/runs/28803586262

[ironloopai]

✅ IronLoop Review: reviewer

Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 8522b3512d343900c230c2a97a17094b59543c61

Run details

Status: Current
Needs human: no
Needs validation: no

**Inline candidates:** 0

Summary

No concrete blocking issues found in the memory context and after-turn recording changes. The PR adds scoped long/short-term memory retrieval, provider-neutral snippet sanitization, native thread-scoped recording, and focused regression coverage.

Findings

None.

Developer follow-up

After fixing this feedback:

1. Push the fix to this PR branch.
2. Re-run this reviewer with @ironloop review --agent reviewer if you only changed this reviewer's findings.
3. Re-run all reviewers with @ironloop review when the fix may affect multiple areas.
4. Use @ironloop status to check queued/running/completed/stale/stalled state while reviewers run.

Superseded by a later IronLoop approved review for this reviewer.

[ironloopai]

✅ IronLoop Review: reviewer

Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: d1f873e49e557d13dd273f38f5afa208dfc0feea

Run details

Status: Current
Needs human: no
Needs validation: no

**Inline candidates:** 0

Summary

No concrete blocking issues found in the reviewed diff. The change adds proactive memory context retrieval and after-turn interaction recording with focused tests around lane ordering, caching, native provider behavior, and runtime wiring.

Findings

None.

Developer follow-up

After fixing this feedback:

1. Push the fix to this PR branch.
2. Re-run this reviewer with @ironloop review --agent reviewer if you only changed this reviewer's findings.
3. Re-run all reviewers with @ironloop review when the fix may affect multiple areas.
4. Use @ironloop status to check queued/running/completed/stale/stalled state while reviewers run.

[BenKurrek]

/canary

[github-actions]

Started Reborn WebUI v2 live canary for reborn/memory-lifecycle at d1f873e49e with cases all: https://github.com/nearai/ironclaw/actions/runs/28807333885