fix: harden memory tool input validation#5701
Conversation
32ed03e to
0f0823f
Compare
0f0823f to
c35cd7e
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMemory tools' input schemas now permit optional ChangesMemory Tools Input Validation
Sequence DiagramsequenceDiagram
participant User
participant RecallMemoryTool
participant Memory
User->>RecallMemoryTool: sends queries (None|string|list)
RecallMemoryTool->>RecallMemoryTool: normalize, trim, filter, dedupe
RecallMemoryTool->>Memory: memory.recall(valid_queries)
Memory-->>RecallMemoryTool: returns matches
RecallMemoryTool-->>User: formatted matches or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/crewai/tests/tools/test_memory_tools.py (1)
50-55: ⚡ Quick winAdd malformed mixed-type input tests to lock in hardening behavior.
Given the hardening objective, consider adding coverage for cases like
queries=["ok", 123]andcontents=["fact", None]to ensure the tools never crash and always return stable validation/success responses.Also applies to: 109-113
🤖 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/tools/test_memory_tools.py` around lines 50 - 55, Add tests that exercise malformed mixed-type inputs so the memory tools never crash and always return stable validation or error responses: extend test_list_of_empty_strings_returns_error (and the analogous test around lines 109-113) to include cases like queries=["ok", 123] and contents=["fact", None], calling RecallMemoryTool._run (and any corresponding tool methods used in the other test) and asserting the call does not raise and returns a predictable error/validation result (e.g., contains "Error" or a stable validation payload) instead of crashing.
🤖 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/tools/memory_tools.py`:
- Around line 54-63: The current guard assumes every item in queries (and
similarly in contents at the other block) is a string and calls .strip(), which
will crash for non-string items like integers or booleans; update the
normalization to first handle non-list inputs, then coerce/filter items to
strings (or filter with isinstance(item, str)) before calling .strip() so
queries = [q for q in queries if isinstance(q, str) and q.strip()] (and apply
the same change to the contents handling around the 115-123 region) ensuring the
function that processes queries/contents rejects or converts non-string entries
rather than raising AttributeError.
---
Nitpick comments:
In `@lib/crewai/tests/tools/test_memory_tools.py`:
- Around line 50-55: Add tests that exercise malformed mixed-type inputs so the
memory tools never crash and always return stable validation or error responses:
extend test_list_of_empty_strings_returns_error (and the analogous test around
lines 109-113) to include cases like queries=["ok", 123] and contents=["fact",
None], calling RecallMemoryTool._run (and any corresponding tool methods used in
the other test) and asserting the call does not raise and returns a predictable
error/validation result (e.g., contains "Error" or a stable validation payload)
instead of crashing.
🪄 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: 4a1b25a8-04e6-464a-bfa4-5abfa951e4df
📒 Files selected for processing (3)
lib/crewai/src/crewai/tools/memory_tools.pylib/crewai/src/crewai/translations/en.jsonlib/crewai/tests/tools/test_memory_tools.py
c35cd7e to
d9d3416
Compare
2f9d683 to
bf40d62
Compare
Add None/empty input handling to RecallMemoryTool and RememberTool. Filter empty strings, convert string inputs to lists, and return descriptive error messages. Update tool descriptions in en.json to include explicit parameter examples. Part 2/4 of Valkey storage implementation.
bf40d62 to
b8cdaf3
Compare
Description:
Part 2/4 of adding Valkey as a storage backend for CrewAI. This PR hardens the memory tools against malformed inputs that surface more frequently with the new storage paths.
What changed:
memory_tools.py — RecallMemoryTool and RememberTool now handle None, empty lists, and lists of empty strings gracefully, returning descriptive error messages instead of crashing. String inputs are normalized to lists. The queries and contents fields are now optional with min_length=1 validation, and both schemas use extra="forbid" to reject unexpected parameters.
en.json — Updated tool descriptions for recall_memory and save_to_memory to include explicit parameter format examples (e.g. {"queries": ["search term"]}), reducing LLM misuse of the tool interface.
Testing:
test_memory_tools.py (15 tests) — Covers None input, empty lists, empty strings, string-to-list conversion, deduplication across queries, and single vs. batch remember paths.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores