GitHub issue #3431: [Reborn] Add MemoryPromptContextService production adapter

[serrrfirat]

Refs #3431

KB task: KB-008

GitHub issue #3431: [Reborn] Add MemoryPromptContextService production adapter

URL: #3431
Repo: nearai/ironclaw
Labels: reborn
Assignees: nickpismenkov

Scope: IronClaw Reborn issue imported from GitHub.
Before coding: read full issue body/comments, check linked PRs/blockers, work from current origin/reborn-integration unless issue says otherwise, keep PR tightly scoped.
Acceptance: satisfy GitHub issue acceptance criteria; include tests/verification evidence; never merge without explicit user approval.

Auto-opened by kb when task reached In Review. Auto-merge remains disabled; do not merge without explicit operator approval.

[gemini-code-assist]

Code Review

This pull request introduces significant architectural updates, including the addition of new crates (ironclaw_conversations, ironclaw_event_projections) and infrastructure for runtime policy planning, memory management, and database-backed state stores. It also adds agent documentation across the workspace and implements new boundary tests. The review feedback highlights several efficiency and scalability opportunities: the sanitize_snippet_text function should be optimized using an iterator-based approach to avoid unnecessary allocations, and the full-state replacement patterns in the libSQL and Postgres storage adapters, as well as the redundant I/O in append_file, should be documented as deferred optimizations with a plan for future incremental improvements.

I am having trouble creating individual review comments. Click here to see my feedback.

crates/ironclaw_host_runtime/src/memory_context.rs (152-182)

The current implementation of sanitize_snippet_text is inefficient for large input strings. It collects all non-control characters into a new String before truncating, which can lead to large unnecessary allocations. To optimize performance, use an iterator-based approach (e.g., chars().by_ref().take(N)) instead of calculating the full length or collecting the entire string beforehand.

References

1. To optimize performance when truncating strings, use an iterator-based approach (e.g., chars().by_ref().take(N)) instead of calculating the full length with chars().count() beforehand.

crates/ironclaw_conversations/src/libsql.rs (535-539)

The save_state_to_conn implementation deletes all rows and re-inserts the entire state. While this snapshot-adapter pattern is acceptable for initial correctness, it should be documented as a deferred optimization. Please add a TODO or documentation noting that incremental updates (e.g., UPSERT) should be implemented as a future task for better scalability.

References

1. If a specific design pattern (e.g., snapshot-adapter) necessitates an inefficient implementation (e.g., loading full state), defer optimization and document the requirement for targeted read paths as a follow-up task.

crates/ironclaw_conversations/src/postgres.rs (532-536)

Similar to the libSQL implementation, save_state_to_txn performs a full state replacement. As this is likely for initial correctness, please document the requirement for incremental persistence as a known future task to avoid performance bottlenecks as data volume grows.

References

1. When an implementation is intentionally inefficient for the sake of initial correctness, document the deferred optimization (e.g., incremental updates) as a known future task.

crates/ironclaw_memory/src/filesystem.rs (304-318)

The append_file implementation results in the document being read twice. If this redundant I/O is accepted for initial correctness, please document this deferred optimization as a known future task. Note that further performance optimizations should be gated on profiling.

References

1. When an implementation is intentionally inefficient for the sake of initial correctness, document the deferred optimization (e.g., incremental updates) as a known future task.
2. Performance optimizations, such as adding a caching layer, should be gated on profiling, especially when existing mechanisms already reduce overhead for common cases.

[zmanian]

Review

Summary: Adds ProductionMemoryPromptContextService bridging the Reborn MemoryBackend into the agent loop's prompt context pipeline, with port trait, empty default, scope-enforced search, deterministic ordering, and LoopSafeSummary-gated sanitization.

Findings:

1. Prompt-injection surface is partially mitigated but coarse (crates/ironclaw_host_runtime/src/memory_context.rs:242-271). Sanitization relies entirely on LoopSafeSummary::new rejecting "path delimiters, sensitive markers, API-key-like tokens". Snippet text is otherwise verbatim. There is no neutralization of instruction-like sequences ("ignore previous instructions", <system> tags, fenced markdown that could hijack tool-call format). Memory content is user-writable and the highest-trust-looking surface to the model — exactly the vector a shared-agent attacker would exploit. Wrap snippets in an explicit untrusted-content envelope (e.g., <memory-snippet untrusted>…</memory-snippet>) or add pattern-based stripping. Not raised by gemini.

2. No total-byte budget. Request carries max_snippets (count) but no token/byte cap. Each snippet up to 512 bytes × 20 = 10KB easily. With no aggregate cap, context profile can blow the model's window. The port should take max_total_bytes or the adapter should track cumulative size.

3. FNV-1a snippet_ref is non-cryptographic (memory_context.rs:215-232), unkeyed. Collisions across tenants are possible — useful only if snippet_ref is later used as a lookup key. Currently appears purely opaque (good); add a comment that it's not collision-resistant and must not be used for authorization.

4. NaN scores silently tie at position 0 (memory_context.rs:138-144). partial_cmp falling back to Ordering::Equal means NaN/Inf inputs reorder arbitrarily. Defensive filter is_nan() or treat as lowest.

5. Context-profile policy is string-matched magic constants (memory_context.rs:191-198) — four variants of "disabled" hardcoded. Violates "strong types over strings". An enum on ContextProfileId or a registry lookup is the right shape; inline TODO acknowledges but should reference a tracked issue.

Verdict: Request changes — (1) and (2) are the asks; (3)–(5) are nits. Scope-filtering, error-redaction, and deterministic ordering are otherwise solid and well-tested.

[serrrfirat]

Addressed the review feedback in ed424062:

• Wrapped accepted memory snippets with an explicit untrusted envelope and drop instruction-like prompt-injection markers before LoopSafeSummary.
• Added aggregate 4 KiB safe-summary budget on top of max_snippets.
• Filtered non-finite scores before deterministic score/path ordering.
• Documented FNV-1a snippet_ref as opaque display-only, not collision-resistant and not valid for auth or backend lookup.
• Reworked disabled context-profile handling through a typed known-profile alias wrapper, with TODO tracked to reborn/#3333.

Validation:

• cargo fmt --check
• cargo test -p ironclaw_host_runtime
• cargo clippy --all --benches --tests --examples --all-features --quiet
• bash scripts/pre-commit-safety.sh
• PR checks classify and scope green

Note: attempted workspace cargo test --lib, but local disk filled (No space left on device) before code failures surfaced. Targeted host-runtime tests and full clippy passed after cleanup.

[zmanian]

Re-review: approved

All five findings resolved at ed424062:

1. Prompt-injection envelope — RESOLVED. UNTRUSTED_MEMORY_PREFIX = "Untrusted memory content: " applied at the sanitize boundary, plus a 17-marker instruction-hijack denylist with word-boundary matching via contains_marker_phrase to avoid false positives. Tests cover both rejection and substring safety.
2. Total-byte budget — RESOLVED. MAX_TOTAL_SAFE_SUMMARY_BYTES = 4 * 1024 enforced in collect_snippets_with_total_budget; test asserts the cap trips before max_snippets.
3. FNV-1a collision docs — RESOLVED. Explicit comment on snippet_ref_for_path: "unkeyed and not collision-resistant, so callers must never use snippet_ref for authorization, tenancy checks, or backend lookup."
4. NaN scores — RESOLVED. results.retain(|r| ... && r.score.is_finite()) before sort, with total_cmp ordering. Test non_finite_scores_are_filtered_before_ordering covers NaN and Infinity.
5. Magic-string policy — PARTIAL. Now wrapped in KnownMemoryContextProfile enum with MEMORY_DISABLED_ALIASES constant and a TODO(reborn/#3333) pointing at the durable context-policy registry. Acknowledged as transitional with a tracked follow-up — acceptable.

Bonus: new error-leak test asserts connection refused / IPs / ports never appear in safe_summary. Good defense in depth.