fix(memory): scope-prefix delete no longer leaks root-scope records or ignores older_than#6535
fix(memory): scope-prefix delete no longer leaks root-scope records or ignores older_than#6535chuenchen309 wants to merge 2 commits into
Conversation
…r ignores older_than
LanceDBStorage.delete()'s scope-prefix branch built the condition as
`scope LIKE '{prefix}%' OR scope = '/'` and joined it with an `older_than`
condition via `" AND ".join(conditions)`. SQL `AND` binds tighter than `OR`,
so this parsed as `scope LIKE 'X%' OR (scope = '/' AND created_at < Y)`:
every record under the target prefix was deleted unconditionally (the age
filter never applied to it), and every unrelated root-scope ('/') record
was deleted regardless of prefix.
Parenthesize the scope clause and drop the unconditional root-scope
inclusion (which the sibling reset() method a few lines below, and
_scan_rows() above, both correctly omit -- confirming this was an
implementation defect, not intentional design).
Added two regression tests against a real local LanceDBStorage table (no
network/embedder needed): one confirms an unrelated root-scope record
survives a prefix-scoped delete, the other confirms older_than actually
filters which records get deleted. Both fail against the pre-fix code and
pass after. Ran the full tests/memory/ suite (149 passed) plus ruff
check/format and mypy (clean).
Disclosure: I used an AI coding assistant (Claude) to help identify this
bug and draft the fix. I independently reproduced both failure modes
against a real local table before writing the tests, and cross-checked
the fix against two other correctly-implemented sibling methods in the
same file before choosing the minimal parenthesization fix over a larger
rewrite.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesLanceDB scoped deletion
🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/crewai/src/crewai/memory/storage/lancedb_storage.py (1)
451-454: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEscape single quotes in the scope prefix to prevent query syntax errors.
If
scope_prefixcontains a single quote, it will break the LanceDB SQL query due to unmatched string literals. Similar to howidis escaped elsewhere in this file using.replace("'", "''"), escaping the prefix is recommended to ensure robust query execution.♻️ Proposed refactor
if not prefix.startswith("/"): prefix = "/" + prefix - conditions.append(f"(scope LIKE '{prefix}%')") + safe_prefix = prefix.replace("'", "''") + conditions.append(f"(scope LIKE '{safe_prefix}%')")🤖 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 451 - 454, Escape single quotes in the scope prefix before constructing the LIKE condition in the scope filtering logic: after normalizing prefix in the scope-prefix handling block, replace each single quote with two single quotes, matching the existing id escaping pattern elsewhere in the storage implementation. Use the escaped value in the generated condition while preserving the current slash normalization.
🤖 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.
Nitpick comments:
In `@lib/crewai/src/crewai/memory/storage/lancedb_storage.py`:
- Around line 451-454: Escape single quotes in the scope prefix before
constructing the LIKE condition in the scope filtering logic: after normalizing
prefix in the scope-prefix handling block, replace each single quote with two
single quotes, matching the existing id escaping pattern elsewhere in the
storage implementation. Use the escaped value in the generated condition while
preserving the current slash normalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e4e148d1-9aa7-4c71-884c-1f8eaca2c148
📒 Files selected for processing (2)
lib/crewai/src/crewai/memory/storage/lancedb_storage.pylib/crewai/tests/memory/test_lancedb_delete_scope.py
Addresses CodeRabbit review feedback on this PR: a scope_prefix
containing a single quote breaks the generated LanceDB SQL query
(unterminated string literal), matching the same class of issue this
file already guards against for record ids via `.replace("'", "''")`.
How verified: added a regression test using a scope containing a
single quote; confirmed it fails with a RuntimeError (lance error:
Unterminated string literal) before the fix, passes after. Full
test_lancedb_delete_scope.py suite (3 tests) passes. ruff, ruff-format,
mypy clean via pre-commit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Good catch — pushed a fix that escapes single quotes in the scope prefix, matching the existing |
|
While cross-checking my other open PRs against this repo's file paths, I found #6459 (opened 2026-07-04, already under active review) fixes the same underlying issue — and more completely: it also covers |
AI-generated contribution disclosure
I used an AI coding assistant (Claude) to help identify this bug and draft the fix. I reproduced both failure modes against a real local
LanceDBStoragetable, wrote/verified the regression tests in both red and green states, cross-checked the fix against two other correctly-implemented sibling methods in the same file, and ran the full local test suite + ruff + mypy before opening this PR. Per CONTRIBUTING.md this should carry thellm-generatedlabel — as an external contributor I don't have permission to apply labels myself (confirmed viagh pr edit --add-label, which returned a GraphQL permission error on my prior PR #6534), so flagging it here for a maintainer to add.Problem
LanceDBStorage.delete()'s scope-prefix branch builds the SQL condition as:SQL
ANDbinds tighter thanOR, so when combined with anolder_thancondition this parses as:Two bugs follow from this:
older_thanage filter never actually applies to it./) is deleted regardless of prefix, as long as it happens to also satisfy the age condition (or if there's no age condition, essentially always).Fix
Parenthesize the scope clause and drop the unconditional root-scope inclusion:
I checked whether the root-scope inclusion might be intentional design, but two sibling code paths in the same file argue against that:
reset()(a few lines below) does prefix-scoped deletion viascope >= '{prefix}' AND scope < '{prefix}/�'with no root-scope carve-out, and_scan_rows()filters with a plainscope LIKE '{prefix}%', also with no such clause. This confirms theOR scope = '/'was an implementation defect, not intentional.Testing
Added
tests/memory/test_lancedb_delete_scope.pywith two regression tests against a real localLanceDBStoragetable (no network/embedder required):test_delete_by_scope_prefix_does_not_delete_unrelated_root_scope_record: saves one root-scope record and one/project-scope record, deletes byscope_prefix="/project", confirms only 1 record is deleted (not both).test_delete_by_scope_prefix_and_older_than_only_deletes_old_records: saves an old and a new record both under/project, deletes withscope_prefix="/project", older_than=<1 day ago>, confirms only the old one is deleted.Both fail against the pre-fix code (2 records deleted instead of 1 in each case, verified via
git stash) and pass after. Ran the fulltests/memory/suite (149 passed),ruff check/ruff format --check, andmypy— all clean.