fix(memory): anchor LanceDB scope matching on path boundary#6459
fix(memory): anchor LanceDB scope matching on path boundary#6459Osamaali313 wants to merge 1 commit into
Conversation
Scope filters in LanceDBStorage matched with a bare `scope LIKE 'prefix%'`
(and, in reset, a lexicographic range `scope >= 'prefix' AND scope <
'prefix/ï¿¿'`). Neither respects the `/` path separator, so a sibling
scope such as `/team-b` matches operations on `/team` because `-` (0x2D)
sorts before `/` (0x2F).
The effect on read paths (search, count, get_scope_info, list_records) is
over-counting; on reset/forget it is data loss -- `reset("/team")` also
deletes every record under the unrelated sibling `/team-b`.
Route all four sites (search, delete, _scan_rows, reset) through a shared
`_scope_prefix_clause` helper that anchors on the boundary -- the exact
scope, or descendants under `prefix + "/"` -- matching the semantics the
qdrant backend and this file's own get_scope_info/list_scopes already use.
The delete path keeps its explicit `scope = '/'` root-record match and the
existing single-quote escaping is preserved.
📝 WalkthroughWalkthroughThis change introduces a boundary-anchored SQL predicate helper, ChangesLanceDB Scope Prefix Matching
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LanceDBStorage
participant ScopePrefixClause as "_scope_prefix_clause"
participant LanceDBTable
Caller->>LanceDBStorage: search/delete/reset(scope_prefix)
LanceDBStorage->>ScopePrefixClause: build predicate(scope_prefix)
ScopePrefixClause-->>LanceDBStorage: SQL predicate string
LanceDBStorage->>LanceDBTable: apply predicate as filter
LanceDBTable-->>LanceDBStorage: matching rows
LanceDBStorage-->>Caller: results
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Note for maintainers: per the contribution policy this PR should carry the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/crewai/tests/memory/test_unified_memory.py (1)
191-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding an underscore-sibling case to guard the LIKE-escaping gap.
The current fixtures (
/team,/team-b) don't exercise LIKE wildcard characters. Adding a scope like/team_aalongside a same-length sibling (e.g./teamZa) would catch the unescaped_wildcard leakage flagged inlancedb_storage.py.🤖 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 `@lib/crewai/tests/memory/test_unified_memory.py` around lines 191 - 199, Add a test case in the unified memory storage suite to cover LIKE wildcard escaping for scope matching: the current count assertions in the memory test only verify slash and hyphen siblings, so extend the fixture around the storage.save and storage.count checks with an underscore-sibling pair such as a scope using /team_a and a same-length sibling like /teamZa. Update the assertions on MemoryRecord scope filtering/count so they verify only the intended descendant scope is included, thereby guarding the escaping behavior in lancedb_storage.py.
🤖 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 `@lib/crewai/src/crewai/memory/storage/lancedb_storage.py`:
- Around line 387-391: The `_scope_prefix_clause` helper only escapes quotes, so
the descendant `LIKE` branch still treats `_` and `%` as wildcards and can match
unrelated scopes. Update the clause-building logic in `lancedb_storage.py` to
escape LIKE metacharacters in `safe` before constructing the `scope LIKE ...`
predicate, and add an explicit `ESCAPE` clause if supported by
LanceDB/DataFusion SQL; otherwise change the descendant matching approach to
avoid wildcard interpretation entirely.
---
Nitpick comments:
In `@lib/crewai/tests/memory/test_unified_memory.py`:
- Around line 191-199: Add a test case in the unified memory storage suite to
cover LIKE wildcard escaping for scope matching: the current count assertions in
the memory test only verify slash and hyphen siblings, so extend the fixture
around the storage.save and storage.count checks with an underscore-sibling pair
such as a scope using /team_a and a same-length sibling like /teamZa. Update the
assertions on MemoryRecord scope filtering/count so they verify only the
intended descendant scope is included, thereby guarding the escaping behavior in
lancedb_storage.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc04b228-e712-4aef-b19c-a2d2139ac0a5
📒 Files selected for processing (2)
lib/crewai/src/crewai/memory/storage/lancedb_storage.pylib/crewai/tests/memory/test_unified_memory.py
| prefix = scope_prefix.rstrip("/") | ||
| if not prefix.startswith("/"): | ||
| prefix = "/" + prefix | ||
| safe = prefix.replace("'", "''") | ||
| return f"(scope = '{safe}' OR scope LIKE '{safe}/%')" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unescaped LIKE wildcards (_, %) can reintroduce the very sibling leakage this PR fixes.
safe only escapes single quotes. In the LIKE branch, _ (single-char wildcard) and % (multi-char wildcard) are still interpreted by the matcher. A scope containing an underscore — common in team/scope names — will match unrelated siblings:
_scope_prefix_clause("/team_x")→scope LIKE '/team_x/%', which also matches/teamAx/child,/team1x/child, etc. (the_matches any single character).%behaves even more broadly.
The exact-match branch is unaffected, but the descendant branch leaks across boundaries, defeating the fix. Escape LIKE metacharacters and add an ESCAPE clause.
🛡️ Proposed fix (escape LIKE metacharacters)
prefix = scope_prefix.rstrip("/")
if not prefix.startswith("/"):
prefix = "/" + prefix
safe = prefix.replace("'", "''")
- return f"(scope = '{safe}' OR scope LIKE '{safe}/%')"
+ # Escape LIKE metacharacters so scopes containing '_' or '%' do not
+ # match unrelated siblings (e.g. '/team_x' must not match '/teamAx/..').
+ like_prefix = (
+ safe.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
+ )
+ return f"(scope = '{safe}' OR scope LIKE '{like_prefix}/%' ESCAPE '\\')"Please confirm LanceDB 0.29.2 (DataFusion SQL) honors the ESCAPE clause on LIKE; if not, the descendant match needs an alternative anchoring approach.
Does LanceDB / DataFusion SQL support the LIKE ESCAPE clause and treat underscore as a single-character wildcard?
🤖 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 `@lib/crewai/src/crewai/memory/storage/lancedb_storage.py` around lines 387 -
391, The `_scope_prefix_clause` helper only escapes quotes, so the descendant
`LIKE` branch still treats `_` and `%` as wildcards and can match unrelated
scopes. Update the clause-building logic in `lancedb_storage.py` to escape LIKE
metacharacters in `safe` before constructing the `scope LIKE ...` predicate, and
add an explicit `ESCAPE` clause if supported by LanceDB/DataFusion SQL;
otherwise change the descendant matching approach to avoid wildcard
interpretation entirely.
There was a problem hiding this comment.
Pull request overview
This PR fixes scope-subtree filtering in LanceDBStorage so that scope operations match only the exact scope and its descendants under prefix + "/", preventing leakage into lexicographic sibling scopes (and avoiding unintended deletes/resets). It also adds a regression test capturing the sibling-scope scenario.
Changes:
- Added a shared
_scope_prefix_clause()helper and routedsearch,delete,_scan_rows, andresetscope filtering through it. - Added a unified-memory regression test ensuring
/teamdoes not include/team-bin counts and thatreset("/team")does not delete/team-b.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/crewai/src/crewai/memory/storage/lancedb_storage.py | Centralizes and updates scope-subtree filtering to anchor on the / path boundary across reads and deletes/resets. |
| lib/crewai/tests/memory/test_unified_memory.py | Adds a regression test covering sibling-scope isolation and reset() behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| prefix = scope_prefix.rstrip("/") | ||
| if not prefix.startswith("/"): | ||
| prefix = "/" + prefix | ||
| safe = prefix.replace("'", "''") | ||
| return f"(scope = '{safe}' OR scope LIKE '{safe}/%')" |
| # count/get_scope_info must include the scope + its descendants only. | ||
| assert storage.count("/team") == 2 # /team and /team/x, NOT /team-b | ||
| assert storage.count("/team-b") == 1 | ||
|
|
||
| # reset must delete only the /team subtree, leaving the sibling intact. | ||
| storage.reset("/team") | ||
| assert storage.count("/team-b") == 1 | ||
| surviving = {r.scope for r in storage.list_records(limit=100)} | ||
| assert surviving == {"/team-b"} |
Problem
Scope filtering in
LanceDBStoragedoes not respect the/path separator, so operations on a scope leak into lexicographic sibling scopes:search,_scan_rows→count/get_scope_info/list_records/list_categories) usedscope LIKE 'prefix%'deleteusedscope LIKE 'prefix%' OR scope = '/'resetused the rangescope >= 'prefix' AND scope < 'prefix/�'Because
-(0x2D) sorts before/(0x2F), a sibling like/team-bmatches an operation scoped to/team.On read paths this over-counts. On
reset/forgetit is data loss:reset("/team")also deletes every record under the unrelated/team-b.Fix
Route all four sites through a shared helper that anchors the match on the path boundary — the exact scope, or descendants under
prefix + "/":This matches the hierarchical semantics the qdrant backend (
_build_scope_filter) and this file's ownget_scope_info/list_scopesalready use (child_prefix = prefix + "/").deletekeeps its explicitscope = '/'root-record match, and the existing single-quote escaping is preserved (orthogonal to #5729).Tests
Added
test_lancedb_scope_prefix_respects_path_boundary(inlib/crewai/tests/memory/test_unified_memory.py), using explicit embeddings so it needs no network. It assertscount("/team") == 2,count("/team-b") == 1, and thatreset("/team")leaves/team-bintact.count("/team")returns3(assert 3 == 2).Contribution note: AI tooling helped surface and reproduce this scope-boundary bug; I reviewed, verified (before/after unit test), and finalized the fix myself. Tagging as
llm-generatedper the contribution policy.