fix(memory): sanitize FTS queries so natural-language recall works on libSQL (#7275) - #7289
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.
The FTS5-keyword probes (AND/OR/NOT) must assert the invocation SUCCEEDS with a literal-token no-match (result_count 0), not result_count 1 — the seeded document does not contain those literal tokens. The regression the probes pin is the hard OperationFailed from FTS5 parsing barewords as operators; that error is gone, and no-match is the correct outcome.
|
🚅 Deployed to the ironclaw-pr-7289 environment in ironclaw-ci-preview
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds production-path memory recall coverage across runtime rebuilds and conversation scopes. It sanitizes native FTS queries and quotes libSQL FTS terms to neutralize operators and embedded quotes. ChangesMemory recall and FTS handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ConversationA
participant MemoryService
participant PersistentStore
participant ConversationB
participant PromptLane
ConversationA->>MemoryService: Persist explicit memory
MemoryService->>PersistentStore: Store and index memory
ConversationB->>MemoryService: Search with sanitized query
MemoryService->>PersistentStore: Execute quoted FTS query
PersistentStore-->>MemoryService: Matching memory
MemoryService-->>ConversationB: Return memory snippets
PromptLane->>MemoryService: Retrieve prompt-context memories
MemoryService->>PersistentStore: Execute scoped FTS query
PersistentStore-->>PromptLane: Matching snippets or empty result
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 |
There was a problem hiding this comment.
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/app/ironclaw_composition/src/factory/tests.rs`:
- Around line 1157-1168: Add a same-user, same-project prompt-lane control
before the existing cross-user and cross-project negative assertions, using the
prompt-lane invocation path and a completed-status record that matches the
in-scope thread or scope. Assert that this control returns a hit, then retain
the existing negative assertions; update the nearby non-vacuity comment to state
that the control covers the prompt lane rather than only memory_search.
In `@crates/substrates/ironclaw_filesystem/src/libsql.rs`:
- Around line 2865-2876: Update fts5_literal_query to reject token-less input,
such as whitespace-only queries, before producing the MATCH expression.
Propagate this outcome through the Filter::Fts translation path as the
established typed Unsupported result (or equivalent empty-result behavior),
ensuring callers cannot bind an empty FTS5 query and that validation does not
rely on MemorySearchRequest::new.
- Line 2840: Make Filter::Fts semantics consistent across backends by defining
the intended query behavior in a shared contract case within the database root
filesystem tests. Exercise syntax such as abc*, col:term, and a OR b, and assert
the same expected row set for both libSQL and Postgres implementations, using
fts5_literal_query and the Postgres Filter::Fts path as the implementation
points to align.
🪄 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: 65e5d3b7-7505-48c6-9de9-0f7f58c96725
📒 Files selected for processing (3)
crates/app/ironclaw_composition/src/factory/tests.rscrates/extensions/packages/memory-native/src/search.rscrates/substrates/ironclaw_filesystem/src/libsql.rs
| // Non-vacuity control: same user + same project, different thread → found. | ||
| let control = invoke_json( | ||
| &services, | ||
| MEMORY_SEARCH_CAPABILITY_ID, | ||
| memory_context_for(MEMORY_SEARCH_CAPABILITY_ID, user, "conv-b", None), | ||
| serde_json::json!({"query": "isolation marker", "limit": 5}), | ||
| ) | ||
| .await | ||
| .expect("control search succeeds"); | ||
| assert_eq!(control["result_count"], serde_json::json!(1)); | ||
|
|
||
| let lane = prompt_lane_service(&services).expect("native binding wires the prompt lane"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The prompt-lane negatives are vacuous, and the doc comment overstates the control.
The control at Line 1158 exercises memory_search only. Lines 1200 and 1232 then assert the prompt lane returns empty for another user and another project. No assertion in this test proves the prompt lane returns anything at all for the in-scope user.
If prompt_lane_service, prompt_lane_turn_scope, or the resolver regressed to always-empty, both negatives still pass and the test reports isolation. The comment at Line 1127 claims the negatives are non-vacuous because of a same-test control; that holds for the search surface, not for the lane.
Add a same-scope lane hit before the negatives. crates/**/*.rs requires caller-level coverage at the nearest meaningful seam, and completed-status-only evidence is insufficient.
💚 Proposed fix: add a prompt-lane control
let lane = prompt_lane_service(&services).expect("native binding wires the prompt lane");
+
+ // Non-vacuity control for the prompt lane itself: same user, same
+ // project, different thread must receive the marker. Without this, the
+ // negative lane assertions below pass even if the lane is always empty.
+ let control_scope = prompt_lane_turn_scope(user, "conv-b", None);
+ let control_snippets = lane
+ .load_memory_snippets(prompt_lane_request(&control_scope, user, "isolation marker?"))
+ .await
+ .expect("control prompt lane retrieval succeeds");
+ assert!(
+ control_snippets
+ .iter()
+ .any(|snippet| snippet.safe_summary.contains(MARKER)),
+ "the in-scope prompt lane must find the marker: {control_snippets:?}"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Non-vacuity control: same user + same project, different thread → found. | |
| let control = invoke_json( | |
| &services, | |
| MEMORY_SEARCH_CAPABILITY_ID, | |
| memory_context_for(MEMORY_SEARCH_CAPABILITY_ID, user, "conv-b", None), | |
| serde_json::json!({"query": "isolation marker", "limit": 5}), | |
| ) | |
| .await | |
| .expect("control search succeeds"); | |
| assert_eq!(control["result_count"], serde_json::json!(1)); | |
| let lane = prompt_lane_service(&services).expect("native binding wires the prompt lane"); | |
| // Non-vacuity control: same user + same project, different thread → found. | |
| let control = invoke_json( | |
| &services, | |
| MEMORY_SEARCH_CAPABILITY_ID, | |
| memory_context_for(MEMORY_SEARCH_CAPABILITY_ID, user, "conv-b", None), | |
| serde_json::json!({"query": "isolation marker", "limit": 5}), | |
| ) | |
| .await | |
| .expect("control search succeeds"); | |
| assert_eq!(control["result_count"], serde_json::json!(1)); | |
| let lane = prompt_lane_service(&services).expect("native binding wires the prompt lane"); | |
| // Non-vacuity control for the prompt lane itself: same user, same | |
| // project, different thread must receive the marker. Without this, the | |
| // negative lane assertions below pass even if the lane is always empty. | |
| let control_scope = prompt_lane_turn_scope(user, "conv-b", None); | |
| let control_snippets = lane | |
| .load_memory_snippets(prompt_lane_request(&control_scope, user, "isolation marker?")) | |
| .await | |
| .expect("control prompt lane retrieval succeeds"); | |
| assert!( | |
| control_snippets | |
| .iter() | |
| .any(|snippet| snippet.safe_summary.contains(MARKER)), | |
| "the in-scope prompt lane must find the marker: {control_snippets:?}" | |
| ); |
🤖 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/app/ironclaw_composition/src/factory/tests.rs` around lines 1157 -
1168, Add a same-user, same-project prompt-lane control before the existing
cross-user and cross-project negative assertions, using the prompt-lane
invocation path and a completed-status record that matches the in-scope thread
or scope. Assert that this control returns a hit, then retain the existing
negative assertions; update the nearby non-vacuity comment to state that the
control covers the prompt lane rather than only memory_search.
Source: Coding guidelines
| }); | ||
| }; | ||
| params.push(libsql::Value::Text(query.clone())); | ||
| params.push(libsql::Value::Text(fts5_literal_query(query))); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Enumerate every Filter::Fts producer and the query values it passes.
set -euo pipefail
rg -nP --type=rust -C5 'Filter::Fts\s*\{' crates/
# 2) Show how the Postgres backend translates Filter::Fts for comparison.
fd -t f 'postgres.*\.rs' crates/substrates/ironclaw_filesystem/src \
--exec rg -nP -C10 'Filter::Fts|plainto_tsquery|websearch_to_tsquery|to_tsquery' {}
# 3) Check whether the shared contract suite covers FTS at all.
fd -t f 'db_root_filesystem_contract.rs' crates/ --exec rg -nP -C4 'Fts|fts' {}Repository: nearai/ironclaw
Length of output: 14642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== libsql FTS helpers/context =="
sed -n '2800,2850p' crates/substrates/ironclaw_filesystem/src/libsql.rs
echo
sed -n '2870,2903p' crates/substrates/ironclaw_filesystem/src/libsql.rs
echo
echo "== memory-native full-text consumer =="
sed -n '920,945p' crates/extensions/packages/memory-native/src/repo/filesystem.rs
echo
echo "== postgres FTS handler =="
sed -n '2440,2466p' crates/substrates/ironclaw_filesystem/src/postgres.rs
echo
echo "== in-memory FTS helper/tests =="
sed -n '780,816p' crates/substrates/ironclaw_filesystem/src/in_memory.rs
sed -n '1528,1570p' crates/substrates/ironclaw_filesystem/src/in_memory.rs
echo
echo "== FTS terms in contract tests around first/last occurrences =="
sed -n '755,785p' crates/substrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rs
sed -n '835,855p' crates/substrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rs
sed -n '3760,3785p' crates/substrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rs
echo
echo "== strict Filter::Fts producer/text matching (non-FullText) =="
rlib='rg -n --type=rust '\'"Filter::Fts\s*{|request\.full_text\(\)' crates/Repository: nearai/ironclaw
Length of output: 11642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== libsql FTS coverage tests =="
sed -n '2900,3075p' crates/substrates/ironclaw_filesystem/src/libsql.rs
echo
echo "== all Filter::Fts queries in tests =="
rg -n -C3 'Filter::Fts\s*\{|full_text\(\)\s*:' crates/substrates/ironclaw_filesystem crates/extensions/packages/memory-native/src/repo/filesystem.rs crates/extensions/packages/memory-native/** 2>/dev/null || true
echo
echo "== fts5_literal_query semantic comparison for representative inputs =="
python3 - <<'PY'
def to_postgres_phrase(s):
return " ".join(f'{token.strip("*")}:*' for token in s.split())
def fts5_literal_query(s):
return " ".join(f'"{token.replace(chr(34), chr(34)+chr(34))}"' for token in s.split())
for s in ["abc*", "col:term", "cat OR dog", "fox \"brown\""]:
print(f"input={s!r}\npostgres={to_postgres_phrase(s)!r}\nlibsql={fts5_literal_query(s)!r}")
PYRepository: nearai/ironclaw
Length of output: 14185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shared FTS contract loop setup =="
sed -n '720,782p' crates/substrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rs
echo
echo "== all Filter::Fts occurrences =="
rg -n -C2 'Filter::Fts\s*{|full_text\(\)\s*:' crates/substrates/ironclaw_filesystem crates/extensions/packages/memory-native/src/repo/filesystem.rs crates/extensions/packages/memory-native 2>/dev/null || true
echo
echo "== backend-specific internal FTS tests =="
rg -n -C4 'fts5_literal_query|fts_.*match|full[_-]?text' crates/substrates/ironclaw_filesystem/src crates/substrates/ironclaw_filesystem/tests 2>/dev/null || trueRepository: nearai/ironclaw
Length of output: 9624
Make Filter::Fts serialization parity explicit across backends.
Filter::Fts is substrate-wide. crates/substrates/ironclaw_filesystem/src/libsql.rs::fts5_literal_query() literals every token before the FTS5 MATCH, while crates/substrates/ironclaw_filesystem/src/postgres.rs passes the raw query into plainto_tsquery. This makes abc*, col:term, and a OR b return different row sets on libSQL versus Postgres, and also diverge from the in-memory naive match. Add a shared contract case covering the intended Filter::Fts semantics in tests/db_root_filesystem_contract.rs so both SQL backends are pinned to one API.
🤖 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/substrates/ironclaw_filesystem/src/libsql.rs` at line 2840, Make
Filter::Fts semantics consistent across backends by defining the intended query
behavior in a shared contract case within the database root filesystem tests.
Exercise syntax such as abc*, col:term, and a OR b, and assert the same expected
row set for both libSQL and Postgres implementations, using fts5_literal_query
and the Postgres Filter::Fts path as the implementation points to align.
Source: Coding guidelines
| /// Render a free-form FTS query as an implicit AND of FTS5 string literals. | ||
| /// | ||
| /// Quoting each whitespace-delimited term keeps uppercase words such as | ||
| /// `AND`, `OR`, and `NOT` from being interpreted as FTS5 syntax. Doubling | ||
| /// embedded quotes follows FTS5's string-literal escaping rules. | ||
| fn fts5_literal_query(query: &str) -> String { | ||
| query | ||
| .split_whitespace() | ||
| .map(|token| format!("\"{}\"", token.replace('"', "\"\""))) | ||
| .collect::<Vec<_>>() | ||
| .join(" ") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Fail closed when the query has no tokens.
If query holds only whitespace, fts5_literal_query returns an empty string. That empty string is bound to MATCH ?, and FTS5 rejects an empty match expression as a syntax error. The caller then sees a Backend error where a typed Unsupported (or an empty result) is the correct outcome.
The memory lane guards this upstream in MemorySearchRequest::new. The filesystem substrate must not depend on one consumer's validator; Filter::Fts is reachable from any domain store.
🛡️ Proposed fix: reject a token-less FTS query in the translator
Filter::Fts { key, query } => {
let Some(fts_table) = fts_tables.get(key.as_str()) else {
return Err(FilesystemError::Unsupported {
path: path.clone(),
operation: FilesystemOperation::Query,
});
};
- params.push(libsql::Value::Text(fts5_literal_query(query)));
+ let match_expression = fts5_literal_query(query);
+ if match_expression.is_empty() {
+ return Err(FilesystemError::Unsupported {
+ path: path.clone(),
+ operation: FilesystemOperation::Query,
+ });
+ }
+ params.push(libsql::Value::Text(match_expression));
out.push_str(&format!(
"(path IN (SELECT path FROM {fts_table} WHERE {fts_table} MATCH ?{}))",
params.len()
));
Ok(())
}🤖 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/substrates/ironclaw_filesystem/src/libsql.rs` around lines 2865 -
2876, Update fts5_literal_query to reject token-less input, such as
whitespace-only queries, before producing the MATCH expression. Propagate this
outcome through the Filter::Fts translation path as the established typed
Unsupported result (or equivalent empty-result behavior), ensuring callers
cannot bind an empty FTS5 query and that validation does not rely on
MemorySearchRequest::new.
Source: Coding guidelines
🔎 Review · PR #7289
Submitted review →Reviewed the complete trusted base-to-head comparison. The memory query normalization and libSQL FTS literalization address punctuation and reserved-keyword failures without weakening scope isolation. The added composition tests cover durable cross-conversation recall, proactive retrieval, isolation, and distinguishable failure behavior. No actionable findings identified. Automatic · PR opened + CI failed · attempt 1 of 3 · completed in 2m 27s Run details
|
There was a problem hiding this comment.
🔍 Review complete · PR #7289
✅ No actionable findings
Reviewed the complete trusted base-to-head comparison. The memory query normalization and libSQL FTS literalization address punctuation and reserved-keyword failures without weakening scope isolation. The added composition tests cover durable cross-conversation recall, proactive retrieval, isolation, and distinguishable failure behavior. No actionable findings identified.
Validation and technical details
- Inspected all three changed files and surrounding memory-native, filesystem backend, and production composition call paths.
- Compared libSQL FTS behavior with the in-memory and PostgreSQL implementations and the shared Filter::Fts contract.
- Verified the exact trusted comparison refs/ironloop/base...refs/ironloop/head and all three commits in that range.
- git diff --check refs/ironloop/base...refs/ironloop/head completed cleanly.
- Repository code graph was unavailable, so review used crate guidance and targeted live-code searches as prescribed.
- Could not execute Rust tests because cargo is not installed in the review environment.
- Base:
main - Head:
implement-issue-7275-testatd646c3b - Run:
5e884d16-0167-4e0a-87cb-1adac4eaaf68
Railway preview QA — PASSTested head: Given/When/Then matrix
Status derivation
Notes
|
CI status: blocked by GitHub Actions outageAll currently-failing checks on head
Local verification on this head: Resume step (once githubstatus shows All Systems Operational): |
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)d646c3bd8(rebased onto current main, incl. #7286)Test 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.