Repository navigation
fix(memory): sanitize FTS queries so natural-language recall works on libSQL (#7275) - #7285
Closed
serrrfirat wants to merge 4 commits into
Closed
serrrfirat wants to merge 4 commits into
serrrfirat wants to merge 4 commits into
Conversation
… libSQL (#7275) Issue #7275 asks for caller-level verification that explicit persistent memory written in conversation A is searchable and proactively recalled in conversation B on the production composition path. Building that verification on the real embedded-libSQL standalone composition exposed a genuine production defect: the libSQL backend passes the memory search query verbatim to FTS5, which treats ?, !, (, ), quotes, and + - * : ^ as operators — any natural-language query containing them makes the MATCH expression invalid, so memory_search fails with OperationFailed and the proactive prompt-memory lane (query = the user's latest message) degrades to empty. User messages almost always carry punctuation, so explicit recall across conversations was unreliable in production. - memory-native: sanitize queries in MemorySearchRequest::new to whitespace-joined alphanumeric tokens (the same treatment the unicode61 tokenizer applies to indexed content); punctuation-only queries still fail as invalid input. - composition factory tests (issue #7275 acceptance criteria 1-6, on the production composition path with the shipping embedded-libSQL backend, not an in-memory harness): - conv A write with provider-issued write evidence (status/path/ content_length), durable across a full runtime teardown/rebuild; - conv B (different thread, same tenant/user/agent/project) finds the marker via memory_search and receives it through the production prompt-memory lane (same derivation build_reborn_runtime wires) without any memory tool call, including natural-language queries with punctuation; - fail-closed for a different user and for an isolated project axis (successful empty results on both surfaces, with non-vacuous same-scope controls); - retrieval failure distinguishable from no-match: no-match is a successful invocation with result_count 0, invalid input fails with InputEncode, backend Unavailable errors degrade the prompt lane to empty while emitting the operator diagnostic (captured via a thread-local subscriber). - regression tests fail without the sanitizer fix.
|
🚅 Deployed to the ironclaw-pr-7285 environment in ironclaw-ci-preview
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7285
August 6, 2026 12:23
Destroyed
12 of 16 tasks
github-actions
Bot
added
size: XL
…he fork PR on the same SHA closed)
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes normalize punctuation-bearing memory searches and add production-path coverage for durable recall, scope isolation, typed failures, prompt-lane degradation, and operator diagnostics. ChangesMemory search and production recall
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/extensions/packages/memory-native/src/search.rs (1)
66-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPunctuation-only input now produces a retrieval failure, which collides with the criterion-5 diagnostic contract.
Two consequences follow from sanitizing before the empty check:
- A query that contains only punctuation, emoji, or CJK punctuation now returns
Err(InvalidId). Before this change it reached the backend. On the prompt lane the caller cannot distinguish this from a backend outage:crates/app/ironclaw_composition/src/factory/tests.rsat lines 1350-1355 asserts the lane emitsmemory context lane retrieval failedfor any error. An operator reading that diagnostic cannot tell "the backend is down" from "the user typed only emoji". The PR states criterion 5 is exactly this distinction.value: queryreports the sanitized value, which is always the empty string on this path. The original input never reaches the error. Keep that behavior if it is intentional redaction, but state it, because the field name implies the offending value.Consider classifying "no searchable tokens" as a distinct outcome so the lane can treat it as a no-match rather than a retrieval failure.
This is a public constructor behavior change. Update the owning memory-search contract documentation.
As per coding guidelines: "Update the owning contract or documentation when behavior changes."
🤖 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/extensions/packages/memory-native/src/search.rs` around lines 66 - 86, Revise the memory-search flow around sanitize_fts_query so punctuation-only, emoji-only, or otherwise tokenless input produces a distinct no-match outcome rather than Err(HostApiError::InvalidId), allowing the prompt lane to distinguish it from backend retrieval failures. Preserve the original query for any error value that still represents invalid input, or explicitly document intentional redaction if sanitized values remain. Update the owning public memory-search contract documentation to describe this behavior.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/app/ironclaw_composition/src/factory/tests.rs`:
- Around line 1282-1304: Update the input-failure case in the caller-level
memory search test to submit a punctuation-only query, exercising the production
sanitizer in MemorySearchRequest::new rather than deserialization of a missing
field, and assert the resulting FailureKind (preserving a separate missing-field
case if its kind differs). Remove the tautological
MemoryServiceError::unavailable() kind assertion, and remove the
MemoryServiceErrorKind import if it is no longer used.
- Around line 1346-1355: Update the diagnostics assertion in the relevant test
to verify the exact error-kind rendering emitted by the ironclaw_host_runtime
diagnostic, in addition to the existing “memory context lane retrieval failed”
message check. Use the error-kind value established by the diagnostic
documentation or implementation so the test fails if that field is omitted.
In `@crates/extensions/packages/memory-native/src/search.rs`:
- Around line 11-31: Update the libSQL backend’s MATCH predicate to quote or
otherwise neutralize uppercase bareword operators produced by
sanitize_fts_query, while leaving sanitize_fts_query’s token output unchanged
for Postgres plainto_tsquery and in-memory substring searches; add caller-level
probes in crates/app/ironclaw_composition/src/factory/tests.rs:1079-1105 for
“AND staging”, “unlocks staging OR”, and “staging NOT”, and update the libSQL
search path in crates/extensions/packages/memory-native/src/search.rs:11-31
accordingly.
---
Outside diff comments:
In `@crates/extensions/packages/memory-native/src/search.rs`:
- Around line 66-86: Revise the memory-search flow around sanitize_fts_query so
punctuation-only, emoji-only, or otherwise tokenless input produces a distinct
no-match outcome rather than Err(HostApiError::InvalidId), allowing the prompt
lane to distinguish it from backend retrieval failures. Preserve the original
query for any error value that still represents invalid input, or explicitly
document intentional redaction if sanitized values remain. Update the owning
public memory-search contract documentation to describe this behavior.
🪄 Autofix
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: 5202f087-9406-4f95-906b-d88cbe2d8629
📒 Files selected for processing (2)
crates/app/ironclaw_composition/src/factory/tests.rscrates/extensions/packages/memory-native/src/search.rs
Collaborator
Author
|
Reopening to trigger a fresh Railway preview deployment for the new head 79f2e5b (deploy-on-open integration). |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7285
August 6, 2026 12:41
Destroyed
Contributor
🔎 Review · PR #7285
1 actionable findings →The sanitizer fixes punctuation-triggered FTS5 errors but still allows alphabetic FTS5 operators, leaving natural-language recall unreliable. The added tests do not cover these reserved tokens. Automatic · PR opened + CI failed · attempt 1 of 3 · completed in 1m 40s Run details
|
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🔍 Review complete · PR #7285
The sanitizer fixes punctuation-triggered FTS5 errors but still allows alphabetic FTS5 operators, leaving natural-language recall unreliable. The added tests do not cover these reserved tokens.
Findings
- 🔴 High · FTS5 keyword operators still pass through sanitization —
crates/extensions/packages/memory-native/src/search.rs:20-22
Details are attached to the relevant diff.
Validation and technical details
- Reviewed the complete trusted comparison refs/ironloop/base (0c297cb) to refs/ironloop/head (79f2e5b): 2 changed files, 617 insertions, 3 deletions.
- Inspected both changed areas, crate-local guidance, MemorySearchRequest call sites, the filesystem repository search path, and libSQL FTS5 MATCH translation.
- Reproduced with SQLite FTS5:
OR,NOT, andANDeach raise syntax errors;what should I NOT forgetis parsed as a boolean expression and returns no match. git diff --check refs/ironloop/base...refs/ironloop/headpassed.- Focused Cargo tests could not be executed because
cargois unavailable in the review environment. - Base:
main - Head:
implement-issue-7275-testat79f2e5b - Run:
1ca9e0a1-88e8-4bec-95e1-4d236b32de8a
Collaborator
Author
|
@ironloopai resolve |
Contributor
🧩 Resolve · PR #7285
Commit ccbd541 →Merged the trusted base without rewriting PR history and committed ccbd541. The update neutralizes FTS5 keyword operators, adds caller-level regression probes, exercises punctuation-only rejection through production composition, removes the tautological assertion, and verifies diagnostic error kinds. Manual command by @serrrfirat · attempt 1 of 3 · completed in 3m 7s Run details
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-7285
August 6, 2026 13:06
Destroyed
16 of 29 tasks
12 of 16 tasks
7 tasks
20 of 28 tasks
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
? ! ( ) " + - * : ^as operators. Any natural-language query containing them makes the MATCH expression invalid:memory_searchfails withOperationFailed, and the proactive prompt-memory lane (whose query is the user's latest message) degrades to empty. User messages almost always carry punctuation — so explicit recall across conversations was unreliable in production. This is the likely root cause of the Memory not reliably recalled across conversations #7185 feedback.MemorySearchRequest::new(memory-native) to whitespace-joined alphanumeric tokens — the same treatment the unicode61 tokenizer applies to indexed content, so it is faithful, not lossy. Punctuation-only queries still fail as invalid input.Change Type
Linked Issue
Closes #7275
Validation
cargo fmt --all -- --checkcargo clippy -p ironclaw_composition -p ironclaw_memory_native --tests(clean)cargo test -p ironclaw_composition --lib— 505 passedcargo test -p ironclaw_memory_native— all suites passed (52+39+13+25+4+4)cargo test -p ironclaw_host_runtime --test memory_prompt_context+ libmemory_*— passedcargo test -p ironclaw_integration_tests --test reborn_group_memory— passedcargo test -p ironclaw_integration_tests --test reborn_integration_wiring_parity— passedreborn_group_multiuserstack-overflows on the base commit too (pre-existing, reproduced with changes stashed)Acceptance-criteria evidence (issue #7275)
memory_recall_across_conversations_on_production_path: tool response carriesstatus:"written",path,append,content_length; tree read-back; full runtime teardown + rebuild over the same libSQL rootmemory_searchresult_count: 1, marker in content,search_scopemarkermemory_context_service(exactmemory_lifecycle_consumersderivationbuild_reborn_runtimewires) returns the marker snippet for conv B's run scope; request mirrors the loop's own buildermemory_recall_fails_closed_for_other_users_and_isolated_scope_axes: cross-user and cross-project both return successfulresult_count: 0and empty prompt lane; non-vacuous same-scope control finds the markermemory_retrieval_failure_is_distinct_from_no_matching_memory+memory_prompt_lane_failure_emits_operator_diagnostic: no-match = successful invocation withresult_count: 0; empty query = failed invocation (InputEncode); backendUnavailable= typedErr(Unavailable), prompt lane degrades to empty while thememory context lane retrieval failedoperator diagnostic is emitted (captured via thread-local subscriber)build_runtime_substrate(local_filesystem_build_input(..))(embedded libSQL, native memory provider)2e2439af4Test Strategy
User behavior: recall of explicitly saved memory across conversations must not depend on whether the user's phrasing carries punctuation.
Risk areas:
Tests added or updated:
MemorySearchRequestsanitizer tests (memory-nativesearch.rs)reborn_group_memory+wiring_parityre-run green)What the tests prove: the full issue #7275 acceptance-criteria matrix above, on the production composition path; plus the FTS5 punctuation defect and its fix.
Commands run: see Validation.