fix(reborn): [PRODUCTION CHANGE] #5605 — wire memory prompt-context source; pin the untrusted-memory envelope at int tier - #5742
henrypark133 wants to merge 10 commits into
Conversation
ProductionMemoryPromptContextService existed but nothing composed it; ThreadBackedLoopContextPort hardcoded memory_snippets: Vec::new(), so the Untrusted memory content: envelope never reached a real system prompt. Mirrors the skill_context_source/identity_context_source sibling shape at every layer: Option<Arc<dyn MemoryPromptContextService>> + setter on ThreadBackedLoopContextPort and RebornLoopDriverHostFactory, mandatory Arc<dyn MemoryPromptContextService> (Empty-fallback) on DefaultPlannedRuntimeParts, and a Some(local_runtime)/None match in build_reborn_runtime identical in shape to the adjacent identity/profile sources — production-graph path stays Empty for the same reason those do. Adds the W4-MEMCTX-ENVELOPE integration test: seeds memory via builtin.memory_write, then asserts a later turn's captured system prompt contains the Untrusted memory content: envelope and the seeded marker; asserts a snippet containing an instruction-hijack marker is dropped by sanitize_snippet_text instead of surfaced. New memory_context_tools() harness profile mirrors profile_tools() (real filesystem-backed MemoryPromptContextService instead of Empty). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
⏳ IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
github-actions
Bot
added
scope: dependencies
📝 WalkthroughSummary by CodeRabbit
WalkthroughWires ChangesMemory prompt-context injection
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: a0f21ddf3cc5e9de2d20061990ce85ae04824c13
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete, actionable regressions found in the memory prompt-context wiring or its integration coverage.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0f21ddf3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let actor = self.run_context.actor().cloned().unwrap_or_else(|| { | ||
| TurnActor::new(self.run_context.scope.to_resource_scope().user_id) | ||
| }); |
There was a problem hiding this comment.
For explicit-owner turns such as shared Slack/team routes, capability execution scopes memory to scope.explicit_owner_user_id() before falling back to the actor, but this lookup always prefers run_context.actor(). When the authenticated actor differs from the explicit thread owner, prompt-context recall searches the actor's memory rather than the conversation owner's memory, so the turn can miss the owner's saved memories and inject another user's memory into this system prompt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: memory recall now prefers scope.explicit_owner_user_id() over run_context.actor(), falling back to the actor only when there's no explicit owner. Added thread_context_port_accepts_explicit_owner_with_distinct_actor's new assertion (a spy source capturing the scoped actor) — mutation-verified RED without the fix.
|
🚅 Deployed to the ironclaw-pr-5742 environment in ironclaw-ci-preview
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Wire Reborn memory prompt-context into production runtime and add integration coverage proving untrusted memory envelope reaches system prompts.
Stats: 5 findings (from 8 raw, 5 after dedupe/filter) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Duplicate suppression: the explicit-owner memory lookup leak is already covered by the live unresolved thread on crates/ironclaw_loop_support/src/lib.rs:406, so I did not repost it.
Security
- Medium User message is used as an unbounded memory search query (
crates/ironclaw_loop_support/src/lib.rs:1885-1891, confidence 75) — anchor:crates/ironclaw_loop_support/src/lib.rs:1890
latest_user_message_queryreturns the full trimmed user message and the new prompt-memory path sends it to FTS on every turn. A large or pathological message can force expensive backend work or backend errors before the model runs.
Tests
-
Medium Memory context lookup errors are not tested (
crates/ironclaw_loop_support/src/lib.rs:407-419, confidence 100) — anchor:crates/ironclaw_loop_support/src/lib.rs:407
The new.await?path propagatesMemoryPromptContextServicefailures, but current tests only cover Empty or successful sources. Also flagged by: performance/Medium. -
Medium Production memory-context composition is not directly tested (
crates/ironclaw_reborn_composition/src/runtime.rs:3499-3508, confidence 75) — anchor:crates/ironclaw_reborn_composition/src/runtime.rs:3499
The new integration test wires memory through the harness helper, not throughbuild_reborn_runtime'slocal_runtime: Somebranch, so a production-composition regression to Empty would not be caught.
Conventions
- Medium Required memory context is still optional in host factory (
crates/ironclaw_reborn/src/loop_driver_host.rs:966-966, confidence 100) — anchor:.claude/rules/architecture.md:64
The factory adds anOption<Arc<dyn MemoryPromptContextService>>plus a production-invoked builder while the runtime parts type already treats the dependency as required. The architecture rule says production-wired runtime dependencies should not stay optional unless they are genuinely optional.
Local Patterns
- Nit Memory wiring hides behind a profile-named filesystem accessor (
tests/integration/support/group.rs:803-805, confidence 50) — anchor:tests/integration/support/harness/recorder.rs:45
The memory-context harness now usescapability_recorder.profile_filesystem(), whose name/docs describe the E-PROFILE seam, making this memory path harder to discover.
| .iter() | ||
| .rev() | ||
| .find(|message| message.kind == MessageKind::User) | ||
| .map(|message| message.content.trim().to_string()) |
There was a problem hiding this comment.
Medium — User message is used as an unbounded memory search query.
latest_user_message_query returns the entire attacker-controlled user message and the new prompt-memory path sends it into memory search on every turn. MemorySearchRequest only rejects empty queries, so a very large or pathological message can force expensive FTS work or backend errors before the model runs.
Fix: Normalize and cap the recall query to a small byte/token budget before invoking memory search, and treat memory lookup failures as no snippets rather than failing the turn.
There was a problem hiding this comment.
Fixed: memory_search_query_from_message bounds the message to MEMORY_QUERY_MAX_CHARS (512) chars and quotes it as a single literal FTS5 phrase so query-syntax metacharacters can't be parsed as backend filters. New unit tests cover quoting, embedded-quote escaping, and length bound — mutation-verified RED.
| .context_profile_id | ||
| .clone(), | ||
| }) | ||
| .await? |
There was a problem hiding this comment.
Medium — Memory context lookup errors are not tested.
This new .await? means a MemoryPromptContextService::load_memory_snippets failure aborts context loading, but the adjacent loop-support tests and the new integration test only cover Empty or successful memory context. That leaves the intended error behavior unpinned.
Fix: Add a ThreadBackedLoopContextPort test with a memory source that returns AgentLoopHostError::Unavailable, and assert the intended behavior, preferably graceful no-snippets fallback for prompt-context recall.
Also flagged by: performance/Medium
There was a problem hiding this comment.
Fixed, and confirmed live in CI: memory recall now degrades to no snippets on a backend error instead of .await? aborting the whole turn — this was the actual root cause of several CI failures (group/root/composition tests, the WebUI v2 E2E smoke, QA recorded-behavior replay all died driver_unavailable under real backend contention before this fix). Added thread_context_port_degrades_to_no_snippets_when_memory_context_source_errors with an always-erroring source; mutation-verified RED against the old fail-closed behavior.
| as Arc<dyn ironclaw_filesystem::RootFilesystem>, | ||
| None, | ||
| )); | ||
| Arc::new(ProductionMemoryPromptContextService::new(memory_service)) |
There was a problem hiding this comment.
Medium — Production memory-context composition is not directly tested.
The new integration test drives RebornIntegrationGroup, whose harness wires memory_context_source through build_memory_context_source_for_test, not through this build_reborn_runtime local_runtime: Some branch. A regression that leaves production composition on EmptyMemoryPromptContextService would not be caught.
Fix: Add a composition/runtime test that builds through build_reborn_runtime(local_dev) and proves the composed memory source reads from the same local runtime memory backing store used by memory_write.
There was a problem hiding this comment.
Added build_reborn_runtime_wires_memory_context_source_when_local_dev in crates/ironclaw_reborn_composition/tests/runtime.rs: drives build_reborn_runtime's own Some(local_runtime) match arm directly (real builtin.memory_write on one conversation, recall asserted via the captured system prompt on a second) rather than the harness's hand-mirrored test-support source. Mutation-verified RED by temporarily forcing that arm to Empty.
| event_subscription: Option<EventTriggeredHookSubscription>, | ||
| safety_context: InstructionSafetyContext, | ||
| identity_context_source: Option<Arc<dyn HostIdentityContextSource>>, | ||
| memory_context_source: Option<Arc<dyn MemoryPromptContextService>>, |
There was a problem hiding this comment.
Medium — Required memory context is still optional in host factory.
This adds memory_context_source: Option<Arc<dyn MemoryPromptContextService>> plus a with_memory_context_source builder, while DefaultPlannedRuntimeParts makes the same dependency required and production composition always calls the builder. .claude/rules/architecture.md says Option<Arc<...>> on a runtime struct is only allowed for genuinely optional dependencies.
Fix: Make memory_context_source required on the factory/context adapter, default helper tests to EmptyMemoryPromptContextService, and remove the dead no-memory branch, or split the type if a true no-memory factory remains legitimate.
There was a problem hiding this comment.
Fixed: memory_context_source on RebornLoopDriverHostFactory is now Arc<dyn MemoryPromptContextService> (mandatory), defaulting to EmptyMemoryPromptContextService in new(), matching user_profile_source's shape; the dead None branch in build is gone.
| // other backends fall back to `EmptyMemoryPromptContextService`. | ||
| memory_context_source: | ||
| ironclaw_reborn_composition::test_support::build_memory_context_source_for_test( | ||
| capability_recorder.profile_filesystem(), |
There was a problem hiding this comment.
Nit — Memory wiring hides behind a profile-named filesystem accessor.
The memory-context harness now consumes capability_recorder.profile_filesystem(), but that accessor name/docs describe the E-PROFILE user-profile seam. Reusing it for memory recall makes this new path harder to find now that the same raw memory filesystem backs more than profile tests.
Fix: Add or rename to a neutral local-dev memory filesystem accessor, update the docs for both profile and memory-context consumers, and keep the profile-specific alias only where it helps profile tests.
There was a problem hiding this comment.
Renamed the recorder's accessor from profile_filesystem() to local_dev_filesystem() and updated its doc to describe both the E-PROFILE and memory-context consumers that now share the one backing store.
…ery bound, fail-open recall, production coverage Codex P1: memory recall now keys off the thread's explicit owner before falling back to the run actor, so shared/explicit-owner threads no longer risk recalling another user's memory. Bounds the recall query derived from the raw user message to MEMORY_QUERY_MAX_CHARS and quotes it as a single literal FTS5 phrase, so conversational query-syntax metacharacters can't be parsed as backend filters and an oversized message can't force unbounded FTS work. Memory recall is best-effort prompt enrichment, unlike the skill/identity sources it sits next to: a `MemoryPromptContextService` error now degrades to no snippets instead of aborting the whole turn via `.await?`. This was live in CI, not just theoretical — every local-dev-composed test now runs the real memory backend on each turn with a user message, and any backend hiccup was failing turns with `driver_unavailable` (group/root/composition tests, the WebUI v2 E2E smoke, and the QA recorded-behavior replay). Fixed the failing/errored-arm test to assert the correct degrade behavior. Adds a composition-tier test that drives build_reborn_runtime's own `Some(local_runtime)` match arm directly (a real memory_write on one conversation, recall asserted in a second), since the existing int-tier harness test only drove a hand-mirrored test-support source. Makes the host-factory's memory_context_source field mandatory (Arc<dyn MemoryPromptContextService>, defaulting to EmptyMemoryPromptContextService) instead of Option, matching the user_profile_source shape and removing a dead always-Some-in-production branch. Renames the harness's profile_filesystem() accessor to local_dev_filesystem() now that both the E-PROFILE and memory-context test consumers share it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🗂️ Archived IronLoop Review: reviewerThis result is from an older PR head and is no longer the active review.
Archived summaryFound one blocking regression in the new memory-context query construction: it quotes every user message as an FTS5 phrase, which is not compatible with the in-memory filesystem FTS implementation used by no-libsql/local test compositions and prevents recalled snippets from matching there. Archived findings
|
github-actions
Bot
added
size: L
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: a9a43b59912b2346a0cdf82e3906316d48c7f20e
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found one blocking regression in the new memory-context query construction: it quotes every user message as an FTS5 phrase, which is not compatible with the in-memory filesystem FTS implementation used by no-libsql/local test compositions and prevents recalled snippets from matching there.
Findings
1. ❌ [MEDIUM] Quoted memory queries do not match the in-memory FTS backend
Location: crates/ironclaw_loop_support/src/lib.rs:1935
memory_search_query_from_message wraps the entire user message in quotes before dispatching to the memory backend. That is FTS5-specific syntax, but the in-memory RootFilesystem backend implements Filter::Fts by splitting the query on whitespace and requiring each token to appear literally. A query like "shipment tracking id" becomes tokens such as "shipment and id", so it will not match stored text without quotes. This breaks memory recall in no-libsql/local in-memory compositions, including the new production-wiring test when ironclaw_reborn_composition is tested without the libsql feature. Keep the backend-neutral query unquoted for backends that do not parse FTS5, or move escaping into the libSQL translator/backend layer.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.36% — 279440 / 327383 lines Per-crate breakdown (65 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 (4 entry/entries excluded from the accounting above)
|
… matcher The bounded, quoted memory-search query loop-support sends now wraps content in a literal `"..."` phrase. The in-memory backend's naive FTS approximation was whitespace-splitting that unconditionally, so the leading/trailing quote characters landed on the first/last token and almost never matched stored text — breaking memory recall for local-dev deployments. Recognize a whole-query double-quoted phrase and match it as literal (unescaped) substring content instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # Cargo.toml
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Wire Reborn memory prompt-context into production composition and add integration coverage for untrusted-memory envelope recall.
Stats: 6 findings (from 11 raw, 6 after dedup/filter) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0. Suppressed duplicate live thread: 1 existing composition coverage thread already had an author reply.
bugs
- Low Empty quoted FTS query matches every in-memory row (
crates/ironclaw_filesystem/src/in_memory.rs:577-578, confidence 75) — anchor: crates/ironclaw_filesystem/src/in_memory.rs:577
fts5_literal_phrasedocuments that an empty quoted phrase should returnNone, but""is accepted and unescaped to an empty string.fts_naive_matchesthen callsstored_lower.contains(""), which is true for every stored text, so an empty quoted FTS query returns all records in the in-memory backend instead of falling back/rejecting like the comment and real FTS behavior imply.
approach
- Medium Loop context now owns an FTS5 query dialect (
crates/ironclaw_loop_support/src/lib.rs:1927-1935, confidence 75) — anchor: crates/ironclaw_filesystem/src/index.rs:292; crates/ironclaw_memory_native/AGENTS.md:18
ThreadBackedLoopContextPortnow turns attacker-controlled message text into an FTS5 double-quoted phrase before calling the memory service, and the PR updates the in-memory filesystem to understand that FTS5 phrase shape. That puts backend query syntax in the loop adapter and generic reference backend even thoughFilter::Ftsis documented as a free-form query each backend translates to its native language, while memory_native owns memory search semantics.
conventions
- Medium Do not make always-wired memory source optional (
crates/ironclaw_loop_support/src/lib.rs:216-216, confidence 100) — anchor: .claude/rules/architecture.md:64
The diff addsmemory_context_source: Option<Arc<dyn MemoryPromptContextService>>plus awith_memory_context_sourcebuilder, but production wiring now owns a requiredArcdefaulting toEmptyMemoryPromptContextServiceand always calls the setter before constructing the loop context. That matches the optional-Arc pattern the architecture rule rejects because theNonebranch is not a real production mode.
maintainability
- Medium Move memory context assembly out of lib.rs (
crates/ironclaw_loop_support/src/lib.rs:404-453, confidence 75) — anchor: crates/ironclaw_loop_support/CLAUDE.md:29
The new memory-recall path adds source selection, actor derivation, backend-query shaping, and best-effort error policy directly inside the already-large thread context adapter. This crate already keeps skill and identity context builders in focused sibling modules, and the crate guide says to add one file per host adapter or context source, so future memory prompt changes now require editing the central loop-support file instead of a narrow owner.
tests
- Medium Memory recall query selection lacks context-window edge tests (
crates/ironclaw_loop_support/src/lib.rs:405-405, confidence 75) — anchor: crates/ironclaw_loop_support/src/lib.rs:405
The new memory lookup path derives a query from a collection of context messages, but the adjacent tests only exercise a single user message. There is no test proving the source is skipped for summary/assistant-only or whitespace-user windows, or that a multi-message window uses the latest user-authored message rather than an older one.
performance
- Low Memory prompt recall over-fetches candidates on every turn (
crates/ironclaw_loop_support/src/lib.rs:426-431, confidence 68) — anchor: crates/ironclaw_memory_native/src/service.rs:331
This new per-turn prompt path asks for 5 memory snippets, but the wiredNativeMemoryService::retrieve_contextpath builds aMemorySearchRequestwith the default pre-fusion limit of 50 before returning at most 5 snippets. That makes every model turn fetch and rank 10x more FTS candidates than the loop can admit.
| max_messages: usize, | ||
| skill_context_source: Option<Arc<dyn HostSkillContextSource>>, | ||
| identity_context_source: Option<Arc<dyn HostIdentityContextSource>>, | ||
| memory_context_source: Option<Arc<dyn MemoryPromptContextService>>, |
There was a problem hiding this comment.
Medium — Do not make always-wired memory source optional.
The diff adds memory_context_source: Option<Arc<dyn MemoryPromptContextService>> plus a with_memory_context_source builder, but production wiring now owns a required Arc defaulting to EmptyMemoryPromptContextService and always calls the setter before constructing the loop context. That matches the optional-Arc pattern the architecture rule rejects because the None branch is not a real production mode.
Fix: Store an Arc<dyn MemoryPromptContextService> on ThreadBackedLoopContextPort, default it to EmptyMemoryPromptContextService or pass it through the constructor, and remove the None branch.
There was a problem hiding this comment.
Fixed in 57a3852: ThreadBackedLoopContextPort.memory_context_source is now a required Arc<dyn MemoryPromptContextService> defaulting to EmptyMemoryPromptContextService in new(); with_memory_context_source is a plain setter and the dead None branch is gone, matching user_profile_source's shape.
| /// metacharacters in ordinary conversational text — hyphens, colons, commas — | ||
| /// can never be parsed as column filters or boolean operators by the | ||
| /// backend's full-text index; they land as literal phrase content instead. | ||
| fn memory_search_query_from_message(content: &str) -> String { |
There was a problem hiding this comment.
Medium — Loop context now owns an FTS5 query dialect.
ThreadBackedLoopContextPort now turns attacker-controlled message text into an FTS5 double-quoted phrase before calling the memory service, and the PR updates the in-memory filesystem to understand that FTS5 phrase shape. That puts backend query syntax in the loop adapter and generic reference backend even though Filter::Fts is documented as a free-form query each backend translates to its native language, while memory_native owns memory search semantics.
Fix: Pass bounded raw text, or a memory-owned literal query type, into the memory service and keep FTS5/libSQL/PostgreSQL escaping in the memory-native or filesystem backend translators.
There was a problem hiding this comment.
Confirmed and restructured in 467a129 — you were right that the loop adapter owned backend dialect. Rather than moving the escaping to another string-shaping layer, literalness is now typed: new Filter::FtsPhrase { key, phrase } variant in ironclaw_filesystem, rendered natively per backend — libSQL owns the FTS5 double-quoted-phrase escaping in its translator, PostgreSQL uses phraseto_tsquery, in-memory does plain substring match (its hand-rolled fts5_literal_phrase parser is deleted entirely). MemorySearchRequest gained literal_phrase; retrieve_context sets it, search() keeps free-form Fts semantics untouched. memory_search_query_from_message is now bounding-only with zero dialect knowledge. Contract tests extended on real libSQL FTS5 + live Postgres with metacharacter/embedded-quote/empty-phrase cases; a facade test pins retrieve-vs-search divergence (reordered tokens excluded by literal phrase, matched by free-form).
| } | ||
| None => Vec::new(), | ||
| }; | ||
| let memory_snippets = match self.memory_context_source.as_deref() { |
There was a problem hiding this comment.
Medium — Move memory context assembly out of lib.rs.
The new memory-recall path adds source selection, actor derivation, backend-query shaping, and best-effort error policy directly inside the already-large thread context adapter. This crate already keeps skill and identity context builders in focused sibling modules, and the crate guide says to add one file per host adapter or context source, so future memory prompt changes now require editing the central loop-support file instead of a narrow owner.
Fix: Create a focused memory_context.rs in ironclaw_loop_support for latest-user query selection, actor selection, query shaping, and the MemoryPromptContextRequest call; have load_loop_context call one helper that returns snippets.
Also flagged by: conventions/Medium
There was a problem hiding this comment.
Fixed in 467a129: extracted crates/ironclaw_loop_support/src/memory_context.rs owning the whole memory-snippets step — latest-user query derivation, explicit-owner actor resolution, best-effort degrade-to-empty error policy, the query bound constants, and their unit tests. lib.rs call site is 6 lines; file went 2282 → 2175 lines.
| None => Vec::new(), | ||
| }; | ||
| let memory_snippets = match self.memory_context_source.as_deref() { | ||
| Some(source) => match latest_user_message_query(&context.messages) { |
There was a problem hiding this comment.
Medium — Memory recall query selection lacks context-window edge tests.
The new memory lookup path derives a query from a collection of context messages, but the adjacent tests only exercise a single user message. There is no test proving the source is skipped for summary/assistant-only or whitespace-user windows, or that a multi-message window uses the latest user-authored message rather than an older one.
Fix: Add thread_context_port_memory_query_uses_latest_user_and_skips_without_user covering multi-message latest-user selection and no-user/blank-user context windows.
There was a problem hiding this comment.
Fixed in 57a3852: one consolidated test thread_context_port_memory_query_uses_latest_user_and_skips_without_user (thread_loop_support_contract.rs) with a query-capturing spy source covers latest-user selection across a [user, assistant, user] window, an assistant-only window skipping recall, and a whitespace-only user message skipping recall. Production latest_user_message_query was already correct — the red run exposed a test-setup error, not a code bug, so only the test scenario was adjusted.
| /// literal content (embedded `""` collapsed to `"`). `None` for anything | ||
| /// else, including an empty or unterminated quote. | ||
| fn fts5_literal_phrase(query: &str) -> Option<String> { | ||
| let inner = query.strip_prefix('"')?.strip_suffix('"')?; |
There was a problem hiding this comment.
Low — Empty quoted FTS query matches every in-memory row.
fts5_literal_phrase documents that an empty quoted phrase should return None, but "" is accepted and unescaped to an empty string. fts_naive_matches then calls stored_lower.contains(""), which is true for every stored text, so an empty quoted FTS query returns all records in the in-memory backend instead of falling back/rejecting like the comment and real FTS behavior imply.
Fix: Reject empty phrase contents before returning from fts5_literal_phrase and add the adjacent empty/unterminated quoted-phrase test.
Also flagged by: tests/Medium, local-patterns/Low
There was a problem hiding this comment.
Fixed in 57a3852 (empty phrase returns None, red→green), then the whole parser was deleted in 467a129 — the FTS5-phrase shape no longer reaches the in-memory backend at all (typed Filter::FtsPhrase instead). The empty-phrase-matches-nothing invariant is re-pinned at the typed seam in fts_phrase_filter_matches_literal_content plus the cross-backend contract tests.
| // a backend hiccup (contention, transient unavailability) | ||
| // degrades to no snippets rather than failing the turn. | ||
| match source | ||
| .load_memory_snippets(MemoryPromptContextRequest { |
There was a problem hiding this comment.
Low — Memory prompt recall over-fetches candidates on every turn.
This new per-turn prompt path asks for 5 memory snippets, but the wired NativeMemoryService::retrieve_context path builds a MemorySearchRequest with the default pre-fusion limit of 50 before returning at most 5 snippets. That makes every model turn fetch and rank 10x more FTS candidates than the loop can admit.
Fix: Set pre_fusion_limit for retrieve_context to a small bounded value derived from max_snippets, mirroring the search path's explicit cap, before disabling vector search.
There was a problem hiding this comment.
Fixed in 57a3852: retrieve_context now sets pre_fusion_limit to max_snippets.saturating_mul(2) (10 for the default 5-snippet prompt path) instead of inheriting the 50-candidate default, mirroring search()'s explicit cap. Pinned by native_context_retrieve_bounds_pre_fusion_limit_relative_to_max_snippets with a spy backend capturing the request limit (red showed 50, green shows 10).
…ion edge tests, empty-phrase FTS guard, bounded pre-fusion limit Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/ironclaw_loop_support/src/lib.rs (1)
405-452: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTwo outstanding architecture comments from prior rounds still apply here.
- Memory-recall assembly (query derivation, actor precedence, service call, error policy) stays inline in
load_loop_context, while skill/identity context building already delegate toskill_context::/identity_context::modules. Per coding guidelines forcrates/ironclaw_loop_support/src/**/*.rs: "Add one file per host adapter or context source." This block should move to a dedicatedmemory_contextmodule mirroring the existing pattern.memory_search_query_from_messagestill bakes FTS5 double-quote phrase escaping into this generic loop-support adapter and hands it straight toMemoryPromptContextRequest.query, coupling the host-agnostic loop port to one backend's query dialect (the in-memory backend was subsequently made FTS5-aware to compensate, rather than moving the dialect ownership to memory-native/backend translators).Both were raised in earlier review rounds without a recorded resolution, and the shipped code still matches the originally reported shape.
🤖 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 405 - 452, The memory-recall logic in load_loop_context should be extracted out of the inline match block into a dedicated memory_context module, following the same host-adapter split already used for skill_context and identity_context. Move the query derivation, actor precedence, load_memory_snippets call, and fallback/error handling into that module, and keep the loop-support adapter focused on orchestration. Also remove the FTS5-specific quote escaping from memory_search_query_from_message and push any backend/query-dialect handling into memory-native/backend translators so MemoryPromptContextRequest.query stays backend-agnostic.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.
Duplicate comments:
In `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 405-452: The memory-recall logic in load_loop_context should be
extracted out of the inline match block into a dedicated memory_context module,
following the same host-adapter split already used for skill_context and
identity_context. Move the query derivation, actor precedence,
load_memory_snippets call, and fallback/error handling into that module, and
keep the loop-support adapter focused on orchestration. Also remove the
FTS5-specific quote escaping from memory_search_query_from_message and push any
backend/query-dialect handling into memory-native/backend translators so
MemoryPromptContextRequest.query stays backend-agnostic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ad80ce2-2b5a-4520-8019-e342a06f6305
📒 Files selected for processing (11)
Cargo.tomlcrates/ironclaw_filesystem/src/in_memory.rscrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_loop_support/tests/thread_loop_support_contract.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/tests/memory_service_facade.rscrates/ironclaw_reborn/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime.rstests/integration/support/group.rstests/integration/support/group_constructors.rstests/integration/wiring_parity.rs
…ntext module Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_loop_support/src/lib.rs (1)
365-403: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider parallelizing the three independent context lookups.
skill_context,identity_context, and nowmemory_context::load_memory_snippets_for_runare three independent async calls awaited sequentially on every turn. Memory recall doesn't depend on skill/identity output (or vice versa), so this is a straightforwardtokio::join!candidate to cut per-turn latency added by this new backend round-trip.⚡ Sketch using tokio::join!
- let instruction_snippets = match self.skill_context_source.as_deref() { - Some(source) => { - skill_context::build_skill_instruction_snippets(source, &self.run_context).await? - } - None => Vec::new(), - }; - let identity_messages = match self.identity_context_source.as_deref() { - Some(source) => { ... } - None => Vec::new(), - }; - let memory_snippets = memory_context::load_memory_snippets_for_run( - &context.messages, - &self.run_context, - self.memory_context_source.as_ref(), - ) - .await; + let skill_fut = async { + match self.skill_context_source.as_deref() { + Some(source) => { + skill_context::build_skill_instruction_snippets(source, &self.run_context).await + } + None => Ok(Vec::new()), + } + }; + let identity_fut = async { /* wrap existing identity branch, returning Result */ }; + let memory_fut = memory_context::load_memory_snippets_for_run( + &context.messages, + &self.run_context, + self.memory_context_source.as_ref(), + ); + let (instruction_snippets, identity_result, memory_snippets) = + tokio::join!(skill_fut, identity_fut, memory_fut); + let instruction_snippets = instruction_snippets?; + let identity_messages = identity_result?;🤖 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 365 - 403, The three independent context fetches in the run flow are still awaited sequentially, which adds avoidable latency. Update the logic in the main turn-building path around skill_context::build_skill_instruction_snippets, identity_context::build_identity_messages_for_run_detailed, and memory_context::load_memory_snippets_for_run to run in parallel with tokio::join! (or equivalent), while preserving the existing per-branch behavior for None cases and the identity_candidates initialization/publish_personal_context_admitted flow.
🤖 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_filesystem/src/libsql.rs`:
- Around line 1805-1821: The libsql FTS phrase handling in the Filter::FtsPhrase
arm currently forwards empty phrases to FTS5 without an explicit check, unlike
in_memory.rs. Add a direct phrase.is_empty() guard before calling
fts5_quote_phrase and building the MATCH clause so empty phrases return
FilesystemError::Unsupported or a no-match path consistent with the other
backend behavior. Use the same FtsPhrase handling in libsql.rs (including the
related arm noted in the review) to keep the empty-phrase contract explicit and
aligned with db_root_filesystem_contract.rs.
In `@crates/ironclaw_filesystem/src/postgres.rs`:
- Around line 1623-1634: The FtsPhrase arm in postgres.rs relies on
phraseto_tsquery handling an empty string implicitly, instead of enforcing the
“empty phrase matches nothing” contract in Rust like in_memory.rs does. Add an
explicit guard in the Filter::FtsPhrase branch before building the SQL so empty
phrases short-circuit to a non-matching predicate, and keep the existing
phraseto_tsquery-based SQL path only for non-empty phrases to match libsql.rs
and in_memory.rs behavior consistently.
---
Outside diff comments:
In `@crates/ironclaw_loop_support/src/lib.rs`:
- Around line 365-403: The three independent context fetches in the run flow are
still awaited sequentially, which adds avoidable latency. Update the logic in
the main turn-building path around
skill_context::build_skill_instruction_snippets,
identity_context::build_identity_messages_for_run_detailed, and
memory_context::load_memory_snippets_for_run to run in parallel with
tokio::join! (or equivalent), while preserving the existing per-branch behavior
for None cases and the identity_candidates
initialization/publish_personal_context_admitted flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e2faae5f-855e-4acc-b569-367e36992d3c
📒 Files selected for processing (11)
crates/ironclaw_filesystem/src/in_memory.rscrates/ironclaw_filesystem/src/index.rscrates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/postgres.rscrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rscrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_loop_support/src/memory_context.rscrates/ironclaw_memory_native/src/repo/filesystem.rscrates/ironclaw_memory_native/src/search.rscrates/ironclaw_memory_native/src/service.rscrates/ironclaw_memory_native/tests/memory_service_facade.rs
| Filter::FtsPhrase { key, phrase } => { | ||
| let Some(fts_table) = fts_tables.get(key.as_str()) else { | ||
| return Err(FilesystemError::Unsupported { | ||
| path: path.clone(), | ||
| operation: FilesystemOperation::Query, | ||
| }); | ||
| }; | ||
| // The dialect boundary: free-form caller text becomes FTS5 query | ||
| // syntax only here, quoted as a single literal phrase so it can | ||
| // never be parsed as column filters or boolean operators. | ||
| params.push(libsql::Value::Text(fts5_quote_phrase(phrase))); | ||
| out.push_str(&format!( | ||
| "(path IN (SELECT path FROM {fts_table} WHERE {fts_table} MATCH ?{}))", | ||
| params.len() | ||
| )); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Empty-phrase "no match" guarantee relies on FTS5 engine behavior, not an explicit guard.
in_memory.rs's FtsPhrase arm explicitly rejects phrase.is_empty() (fixing a prior flagged bug where contains("") matched everything). This libsql arm has no equivalent guard — it always quotes and forwards the phrase to MATCH, relying on SQLite FTS5 to naturally return zero rows for a MATCH '""' empty phrase rather than erroring or matching broadly. The empty-phrase contract test in db_root_filesystem_contract.rs presumably validates current behavior, but making the "empty phrase → no match" contract explicit in Rust (rather than delegated to FTS5's undocumented-for-this-edge-case behavior) would keep the three backends' semantics visibly aligned and immune to any future libsql/SQLite behavior change.
♻️ Proposed defensive guard
Filter::FtsPhrase { key, phrase } => {
+ if phrase.is_empty() {
+ out.push_str("FALSE");
+ return Ok(());
+ }
let Some(fts_table) = fts_tables.get(key.as_str()) else {Since this touches SQLite FTS5 dialect behavior for an edge case, please confirm MATCH '""' reliably returns zero rows (rather than erroring) across the libsql/SQLite version(s) this crate targets.
Also applies to: 1886-1893
🤖 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_filesystem/src/libsql.rs` around lines 1805 - 1821, The
libsql FTS phrase handling in the Filter::FtsPhrase arm currently forwards empty
phrases to FTS5 without an explicit check, unlike in_memory.rs. Add a direct
phrase.is_empty() guard before calling fts5_quote_phrase and building the MATCH
clause so empty phrases return FilesystemError::Unsupported or a no-match path
consistent with the other backend behavior. Use the same FtsPhrase handling in
libsql.rs (including the related arm noted in the review) to keep the
empty-phrase contract explicit and aligned with db_root_filesystem_contract.rs.
| Filter::FtsPhrase { key, phrase } => { | ||
| // `phraseto_tsquery` requires the lexemes it finds to appear as | ||
| // an exact adjacent phrase, giving literal-phrase semantics | ||
| // instead of `plainto_tsquery`'s implicit AND-of-terms. | ||
| params.push(Box::new(phrase.clone())); | ||
| out.push_str(&format!( | ||
| "(to_tsvector('english', COALESCE(indexed->>'{}', '')) @@ phraseto_tsquery('english', ${}))", | ||
| key.as_str(), | ||
| params.len() | ||
| )); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Same empty-phrase reliance as libsql — consider an explicit guard for parity.
Mirrors the concern raised on libsql.rs's FtsPhrase arm: this relies on phraseto_tsquery('english', '') reducing to an empty tsquery that never matches via @@, rather than an explicit Rust-level guard. Since in_memory.rs enforces "empty phrase matches nothing" explicitly, doing the same here keeps all three backends' contracts visibly identical instead of two of them depending on FTS-engine-specific empty-input behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_filesystem/src/postgres.rs` around lines 1623 - 1634, The
FtsPhrase arm in postgres.rs relies on phraseto_tsquery handling an empty string
implicitly, instead of enforcing the “empty phrase matches nothing” contract in
Rust like in_memory.rs does. Add an explicit guard in the Filter::FtsPhrase
branch before building the SQL so empty phrases short-circuit to a non-matching
predicate, and keep the existing phraseto_tsquery-based SQL path only for
non-empty phrases to match libsql.rs and in_memory.rs behavior consistently.
|
Closing as stale — no activity in over three weeks. The branch is untouched; reopen if this is still needed. |
Root cause
ProductionMemoryPromptContextService(crates/ironclaw_host_runtime/src/memory_context.rs) was fully implemented — including thesanitize_snippet_text/wrap_untrusted_with_limitprompt-injection-hardening envelope — but nothing inironclaw_reborn_compositioncomposed it. The only productionLoopContextPortimplementation,ThreadBackedLoopContextPort, hardcodedmemory_snippets: Vec::new()(crates/ironclaw_loop_support/src/lib.rs). Unlike itsskill_context_source/identity_context_sourcesiblings, there was nowith_memory_context_sourcesetter at all, so every real Reborn turn's memory recall was silently inert and theUntrusted memory content:envelope never reached a real system prompt.Wiring shape
Mirrors the
skill_context_source/identity_context_sourcesibling pattern exactly, at every layer:ThreadBackedLoopContextPort(ironclaw_loop_support): newmemory_context_source: Option<Arc<dyn MemoryPromptContextService>>field +with_memory_context_sourcesetter.load_loop_contextderives the search query from the most recent user-authored message in the loaded context window (perMemoryPromptContextRequest's own doc contract) and skips the lookup entirely when there's no user message — same graceful-degrade shape as the skill/identityNonebranches.RebornLoopDriverHostFactory(ironclaw_reborn): sameOptionfield/setter, wired intoThreadBackedLoopContextPortconstruction right next to the existing skill/identity conditionals.DefaultPlannedRuntimeParts(ironclaw_reborn::runtime): mandatoryArc<dyn MemoryPromptContextService>field (mirrorsidentity_context_source's required-with-Empty-fallback shape), unconditionally threaded throughbuild_default_planned_runtime_inner.build_reborn_runtime(ironclaw_reborn_composition::runtime):match local_runtime { Some(rt) => real, None => Empty }, identical in shape to the adjacentidentity_context_source/user_profile_sourceblock. The real branch composesProductionMemoryPromptContextServiceover a freshNativeMemoryServicebacked by the samelocal_runtime.extension_filesystemMemoryBackedUserProfileSourcealready reads from — so abuiltin.memory_writeand this recall path see the same backing store. The production-graph path (local_runtime: None) staysEmpty, matching the pre-existing, already-commented precedent that identity/profile sources are deferred there too — not a new gap this PR introduces.Profile preservation
Every existing composition profile keeps its exact current behavior:
Emptyinlocal_runtime: Nonecase is unchanged, and theSomecase only adds new (previously-absent) behavior. ~8 existingDefaultPlannedRuntimePartstest-literal call sites were updated to explicitly supplyArc::new(EmptyMemoryPromptContextService), forced by the compiler — mechanical, behavior-preserving.Test — W4-MEMCTX-ENVELOPE (int tier)
New
tests/integration/memory_prompt_context.rs, driven through a newmemory_context_tools()harness group (mirrorsprofile_tools(), since it needs the same real local-dev filesystemlocal_dev_profile_filesystem_for_test()exposes):builtin.memory_writeon one thread, then on a fresh thread (no tool call) asserts the captured system prompt containsUntrusted memory content:and the seeded marker — proving the envelope reaches the model on every turn, not only when a memory tool is explicitly called.ignore previous instructions) is dropped entirely bysanitize_snippet_textrather than escaped — asserted by confirming its content never surfaces, enveloped or otherwise.EmptyMemoryPromptContextService— confirmed RED for the right reason (empty snippets) — then restored, confirmed GREEN.Gates:
cargo fmtclean;cargo clippy --all --benches --tests --examples --all-features— 0 warnings;cargo test -p ironclaw_loop_support --lib— 352 passed;cargo test -p ironclaw_reborn --lib— 301 passed;cargo test -p ironclaw_reborn_composition --lib— 908 passed; touched int suites (reborn_integration_memory_prompt_context,reborn_integration_wiring_parity,reborn_integration_profile,reborn_direct_chat_user_scope_isolation_parity,reborn_response_order_parity) all green; crate-tier test binaries at every touched call site (ironclaw_product_workflow::inbound_turn_contract,ironclaw_reborn::loop_driver_host,ironclaw_reborn_composition::product_live_adapters) all green.Closes #5605
🤖 Generated with Claude Code