feat(knowledge): add metadata_filter config and metadata save support - #6958
Open
jpcj223 wants to merge 1 commit into
Open
feat(knowledge): add metadata_filter config and metadata save support#6958jpcj223 wants to merge 1 commit into
jpcj223 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughKnowledge metadata filters are added to configuration and synchronous/asynchronous query APIs. Knowledge sources now pass metadata to storage, and stored documents retain non-empty metadata. Tests cover configuration, queries, persistence, and source propagation. ChangesKnowledge Metadata Support
Sequence Diagram(s)sequenceDiagram
participant Crew
participant Knowledge
participant KnowledgeStorage
Crew->>Knowledge: query(query, metadata_filter)
Knowledge->>KnowledgeStorage: search(query, metadata_filter)
Crew->>Knowledge: aquery(query, metadata_filter)
Knowledge->>KnowledgeStorage: asearch(query, metadata_filter)
Suggested reviewers: 🚥 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 Warning |
![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
🧹 Nitpick comments (1)
lib/crewai/src/crewai/crew.py (1)
2061-2093: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new public metadata parameters.
The new Crew query parameters and storage save parameters are public API. Their documentation does not describe
metadata_filterormetadataconsistently.
lib/crewai/src/crewai/crew.py#L2061-L2093: addArgsentries formetadata_filterto both Crew query methods.lib/crewai/src/crewai/knowledge/storage/base_knowledge_storage.py#L38-L50: documentdocumentsandmetadatain the abstract storage contract.lib/crewai/src/crewai/knowledge/storage/knowledge_storage.py#L105-L125: add asavedocstring that documents metadata attachment behavior.As per coding guidelines,
**/*.py: Document public APIs and complex logic.🤖 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/crew.py` around lines 2061 - 2093, Document the public metadata parameters at all affected sites: in lib/crewai/src/crewai/crew.py lines 2061-2093, add Args entries describing metadata_filter to both query_knowledge and aquery_knowledge; in lib/crewai/src/crewai/knowledge/storage/base_knowledge_storage.py lines 38-50, document the documents and metadata parameters in the abstract storage contract; and in lib/crewai/src/crewai/knowledge/storage/knowledge_storage.py lines 105-125, add a save docstring explaining metadata attachment 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 `@lib/crewai/src/crewai/knowledge/storage/base_knowledge_storage.py`:
- Around line 38-50: Preserve compatibility with storage implementations that
still expose save(documents) and asave(documents): in base_knowledge_storage.py,
define the migration policy for optional metadata, and update the storage
invocation paths to omit the metadata keyword when it is None/empty or route
through a compatibility adapter. Apply the corresponding call-site changes in
base_knowledge_source.py lines 62-85 and base_file_knowledge_source.py lines
74-87; ensure metadata is still passed whenever supplied and legacy
implementations do not receive the keyword.
In `@lib/crewai/tests/knowledge/test_knowledge_metadata.py`:
- Around line 43-117: Extend
lib/crewai/tests/knowledge/test_knowledge_metadata.py at lines 43-117 with sync
and async Crew.query_knowledge()/aquery_knowledge() tests using a mocked
Knowledge instance, asserting metadata_filter is forwarded. At lines 209-258,
add sync and async BaseFileKnowledgeSource persistence tests that verify
metadata reaches storage; keep the tests focused on observable behavior.
- Around line 96-101: Update
lib/crewai/tests/knowledge/test_knowledge_metadata.py lines 96-101 so
mock_storage.asearch uses an AsyncMock with the existing SearchResult list as
its return value; also update lines 188-191 so the awaited
aget_or_create_collection and aadd_documents methods use AsyncMock instances
configured with their existing return values.
---
Nitpick comments:
In `@lib/crewai/src/crewai/crew.py`:
- Around line 2061-2093: Document the public metadata parameters at all affected
sites: in lib/crewai/src/crewai/crew.py lines 2061-2093, add Args entries
describing metadata_filter to both query_knowledge and aquery_knowledge; in
lib/crewai/src/crewai/knowledge/storage/base_knowledge_storage.py lines 38-50,
document the documents and metadata parameters in the abstract storage contract;
and in lib/crewai/src/crewai/knowledge/storage/knowledge_storage.py lines
105-125, add a save docstring explaining metadata attachment 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c07b39c-bd89-4d83-8b62-622c08838d45
📒 Files selected for processing (8)
lib/crewai/src/crewai/crew.pylib/crewai/src/crewai/knowledge/knowledge.pylib/crewai/src/crewai/knowledge/knowledge_config.pylib/crewai/src/crewai/knowledge/source/base_file_knowledge_source.pylib/crewai/src/crewai/knowledge/source/base_knowledge_source.pylib/crewai/src/crewai/knowledge/storage/base_knowledge_storage.pylib/crewai/src/crewai/knowledge/storage/knowledge_storage.pylib/crewai/tests/knowledge/test_knowledge_metadata.py
Comment on lines
+38
to
+50
| def save( | ||
| self, | ||
| documents: list[str], | ||
| metadata: dict[str, Any] | None = None, | ||
| ) -> None: | ||
| """Save documents to the knowledge base.""" | ||
|
|
||
| @abstractmethod | ||
| async def asave(self, documents: list[str]) -> None: | ||
| async def asave( | ||
| self, | ||
| documents: list[str], | ||
| metadata: dict[str, Any] | None = None, | ||
| ) -> None: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find in-repository BaseKnowledgeStorage implementations and inspect save signatures.
ast-grep outline . --items all --type class --match 'KnowledgeStorage'
rg -n -U --glob '*.py' \
'class\s+\w+\([^)]*\bBaseKnowledgeStorage\b[^)]*\):(?s:.*?)^\s*(?:async\s+def\s+)?a?save\(' .Repository: crewAIInc/crewAI
Length of output: 20533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
root = Path("lib/crewai")
targets = [
root / "src/crewai/knowledge/storage/base_knowledge_storage.py",
root / "src/crewai/knowledge/source/base_knowledge_source.py",
root / "src/crewai/knowledge/source/base_file_knowledge_source.py",
]
print("TARGET CALLS")
for path in targets:
print(f"\n== {path} ==")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "storage.save(" in line or "storage.asave(" in line:
print(f"{i}: {line.strip()}")
for j in range(i, min(i + 6, len(lines) + 1)):
print(f"{j}: {lines[j-1]}")
print("\nIN-REPOSITORY SUBCLASS SIGNATURES")
for path in root.rglob("*.py"):
try:
tree = ast.parse(path.read_text())
except SyntaxError:
continue
for node in ast.walk(tree):
if isinstance(node, ast.ClassDef):
bases = [ast.unparse(base) for base in node.bases]
if any("BaseKnowledgeStorage" in base for base in bases):
print(f"\n{path}:{node.lineno} {node.name}({', '.join(bases)})")
for item in node.body:
if isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)) and item.name in {"save", "asave"}:
print(f" {item.lineno}: {ast.unparse(item)}")
print("\nLEGACY CALL PROBE")
class LegacyStorage:
def save(self, documents):
return None
async def asave(self, documents):
return None
legacy = LegacyStorage()
for method, args in [
(legacy.save, (["doc"],)),
(legacy.save, (["doc"],)),
]:
method(*args)
print("positional calls succeed")
try:
legacy.save(["doc"], metadata=None)
except TypeError as exc:
print(f"save(metadata=None) -> {type(exc).__name__}: {exc}")
import asyncio
async def probe():
try:
await legacy.asave(["doc"], metadata=None)
except TypeError as exc:
print(f"asave(metadata=None) -> {type(exc).__name__}: {exc}")
asyncio.run(probe())
PYRepository: crewAIInc/crewAI
Length of output: 4867
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
root = Path("lib/crewai")
targets = [
root / "src/crewai/knowledge/storage/base_knowledge_storage.py",
root / "src/crewai/knowledge/source/base_knowledge_source.py",
root / "src/crewai/knowledge/source/base_file_knowledge_source.py",
]
print("TARGET CALLS")
for path in targets:
print(f"\n== {path} ==")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "storage.save(" in line or "storage.asave(" in line:
for j in range(i, min(i + 7, len(lines) + 1)):
print(f"{j}: {lines[j-1]}")
print("\nIN-REPOSITORY SUBCLASS SIGNATURES")
for path in root.rglob("*.py"):
try:
tree = ast.parse(path.read_text())
except SyntaxError:
continue
for node in ast.walk(tree):
if isinstance(node, ast.ClassDef):
bases = [ast.unparse(base) for base in node.bases]
if any("BaseKnowledgeStorage" in base for base in bases):
print(f"{path}:{node.lineno} {node.name}")
for item in node.body:
if isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)):
if item.name in {"save", "asave"}:
print(f" {item.lineno}: {ast.unparse(item)}")
print("\nLEGACY CALL PROBE")
class LegacyStorage:
def save(self, documents):
return None
async def asave(self, documents):
return None
legacy = LegacyStorage()
legacy.save(["doc"])
print("positional save succeeds")
try:
legacy.save(["doc"], metadata=None)
except TypeError as exc:
print(f"save(metadata=None) -> {type(exc).__name__}: {exc}")
import asyncio
async def probe():
try:
await legacy.asave(["doc"], metadata=None)
except TypeError as exc:
print(f"asave(metadata=None) -> {type(exc).__name__}: {exc}")
asyncio.run(probe())
PYRepository: crewAIInc/crewAI
Length of output: 4707
Preserve compatibility with legacy storage implementations.
Both source helpers pass metadata=None, so implementations with the previous save(documents) or asave(documents) signatures raise TypeError. Define a migration policy and either omit the keyword when metadata is empty or provide a compatibility adapter for legacy storage implementations.
📍 Affects 3 files
lib/crewai/src/crewai/knowledge/storage/base_knowledge_storage.py#L38-L50(this comment)lib/crewai/src/crewai/knowledge/source/base_knowledge_source.py#L62-L85lib/crewai/src/crewai/knowledge/source/base_file_knowledge_source.py#L74-L87
🤖 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/knowledge/storage/base_knowledge_storage.py` around
lines 38 - 50, Preserve compatibility with storage implementations that still
expose save(documents) and asave(documents): in base_knowledge_storage.py,
define the migration policy for optional metadata, and update the storage
invocation paths to omit the metadata keyword when it is None/empty or route
through a compatibility adapter. Apply the corresponding call-site changes in
base_knowledge_source.py lines 62-85 and base_file_knowledge_source.py lines
74-87; ensure metadata is still passed whenever supplied and legacy
implementations do not receive the keyword.
Comment on lines
+43
to
+117
| class TestKnowledgeQueryMetadata: | ||
| """Tests for Knowledge.query() / aquery() forwarding metadata_filter.""" | ||
|
|
||
| def test_query_forwards_metadata_filter_to_storage(self): | ||
| """Knowledge.query() should pass metadata_filter to storage.search().""" | ||
| mock_storage = MagicMock() | ||
| mock_storage.search.return_value = [ | ||
| SearchResult( | ||
| id="1", content="test content", metadata={"env": "prod"}, score=0.9 | ||
| ) | ||
| ] | ||
|
|
||
| knowledge = Knowledge( | ||
| collection_name="test", | ||
| sources=[], | ||
| ) | ||
| knowledge.storage = mock_storage | ||
|
|
||
| metadata_filter = {"env": "prod", "category": "tech"} | ||
| knowledge.query( | ||
| ["test query"], | ||
| results_limit=5, | ||
| score_threshold=0.5, | ||
| metadata_filter=metadata_filter, | ||
| ) | ||
|
|
||
| mock_storage.search.assert_called_once() | ||
| call_kwargs = mock_storage.search.call_args | ||
| assert call_kwargs.kwargs.get("metadata_filter") == metadata_filter | ||
| assert call_kwargs.kwargs.get("limit") == 5 | ||
| assert call_kwargs.kwargs.get("score_threshold") == 0.5 | ||
|
|
||
| def test_query_without_metadata_filter_passes_none(self): | ||
| """Knowledge.query() without metadata_filter should pass None.""" | ||
| mock_storage = MagicMock() | ||
| mock_storage.search.return_value = [] | ||
|
|
||
| knowledge = Knowledge(collection_name="test", sources=[]) | ||
| knowledge.storage = mock_storage | ||
|
|
||
| knowledge.query(["test query"]) | ||
|
|
||
| mock_storage.search.assert_called_once() | ||
| call_kwargs = mock_storage.search.call_args | ||
| assert call_kwargs.kwargs.get("metadata_filter") is None | ||
|
|
||
| @pytest.mark.skipif( | ||
| sys.platform == "win32", | ||
| reason="Async tests fail on Windows due to pytest-recording + asyncio event loop compatibility", | ||
| ) | ||
| @pytest.mark.asyncio | ||
| async def test_aquery_forwards_metadata_filter_to_storage(self): | ||
| """Knowledge.aquery() should pass metadata_filter to storage.asearch().""" | ||
| mock_storage = MagicMock() | ||
| mock_storage.asearch.return_value = [ | ||
| SearchResult( | ||
| id="1", content="test content", metadata={"env": "prod"}, score=0.9 | ||
| ) | ||
| ] | ||
|
|
||
| knowledge = Knowledge(collection_name="test", sources=[]) | ||
| knowledge.storage = mock_storage | ||
|
|
||
| metadata_filter = {"status": "active"} | ||
| await knowledge.aquery( | ||
| ["test query"], | ||
| results_limit=3, | ||
| score_threshold=0.7, | ||
| metadata_filter=metadata_filter, | ||
| ) | ||
|
|
||
| mock_storage.asearch.assert_called_once() | ||
| call_kwargs = mock_storage.asearch.call_args | ||
| assert call_kwargs.kwargs.get("metadata_filter") == metadata_filter | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add behavior tests for the remaining changed metadata paths.
The suite does not invoke Crew.query_knowledge() or Crew.aquery_knowledge(). It also does not invoke BaseFileKnowledgeSource persistence methods. Regressions in these changed paths will not fail a test.
lib/crewai/tests/knowledge/test_knowledge_metadata.py#L43-L117: add sync and async Crew forwarding tests with a mockedKnowledgeinstance.lib/crewai/tests/knowledge/test_knowledge_metadata.py#L209-L258: add sync and async file-source tests that assert metadata reaches storage.
As per coding guidelines, **/*test*.py: Write unit tests for new functionality, focusing on behavior rather than implementation details.
📍 Affects 1 file
lib/crewai/tests/knowledge/test_knowledge_metadata.py#L43-L117(this comment)lib/crewai/tests/knowledge/test_knowledge_metadata.py#L209-L258
🤖 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/knowledge/test_knowledge_metadata.py` around lines 43 - 117,
Extend lib/crewai/tests/knowledge/test_knowledge_metadata.py at lines 43-117
with sync and async Crew.query_knowledge()/aquery_knowledge() tests using a
mocked Knowledge instance, asserting metadata_filter is forwarded. At lines
209-258, add sync and async BaseFileKnowledgeSource persistence tests that
verify metadata reaches storage; keep the tests focused on observable behavior.
Source: Coding guidelines
Comment on lines
+96
to
+101
| mock_storage = MagicMock() | ||
| mock_storage.asearch.return_value = [ | ||
| SearchResult( | ||
| id="1", content="test content", metadata={"env": "prod"}, score=0.9 | ||
| ) | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use AsyncMock for awaited storage methods.
Knowledge.aquery() awaits mock_storage.asearch(). The test configures it to return a list. KnowledgeStorage.asave() awaits both client methods. The test configures them to return None. These tests raise TypeError on non-Windows platforms.
lib/crewai/tests/knowledge/test_knowledge_metadata.py#L96-L101: replacemock_storage.asearchwithAsyncMock(return_value=...).lib/crewai/tests/knowledge/test_knowledge_metadata.py#L188-L191: replaceaget_or_create_collectionandaadd_documentswithAsyncMockinstances.
Proposed fix
-from unittest.mock import MagicMock, patch
+from unittest.mock import AsyncMock, MagicMock, patch
- mock_storage.asearch.return_value = [
+ mock_storage.asearch = AsyncMock(return_value=[
SearchResult(
id="1", content="test content", metadata={"env": "prod"}, score=0.9
)
- ]
+ ])
- mock_client.aget_or_create_collection.return_value = None
- mock_client.aadd_documents.return_value = None
+ mock_client.aget_or_create_collection = AsyncMock(return_value=None)
+ mock_client.aadd_documents = AsyncMock(return_value=None)📝 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.
Suggested change
| mock_storage = MagicMock() | |
| mock_storage.asearch.return_value = [ | |
| SearchResult( | |
| id="1", content="test content", metadata={"env": "prod"}, score=0.9 | |
| ) | |
| ] | |
| mock_storage = MagicMock() | |
| mock_storage.asearch = AsyncMock(return_value=[ | |
| SearchResult( | |
| id="1", content="test content", metadata={"env": "prod"}, score=0.9 | |
| ) | |
| ]) |
📍 Affects 1 file
lib/crewai/tests/knowledge/test_knowledge_metadata.py#L96-L101(this comment)lib/crewai/tests/knowledge/test_knowledge_metadata.py#L188-L191
🤖 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/knowledge/test_knowledge_metadata.py` around lines 96 - 101,
Update lib/crewai/tests/knowledge/test_knowledge_metadata.py lines 96-101 so
mock_storage.asearch uses an AsyncMock with the existing SearchResult list as
its return value; also update lines 188-191 so the awaited
aget_or_create_collection and aadd_documents methods use AsyncMock instances
configured with their existing return values.
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
1 participant
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.
The lower layers of the knowledge system already support metadata —
KnowledgeStorage.search()acceptsmetadata_filter,BaseRecordhas ametadatafield, andBaseKnowledgeSourcehas ametadatafield. However, 4 gaps prevent users from actually using metadata from user-land:KnowledgeConfig— nometadata_filterfield, so metadata-based retrieval cannot be configured at the config/agent levelKnowledge.query()/Knowledge.aquery()— don't forwardmetadata_filterto storageCrew.query_knowledge()/Crew.aquery_knowledge()— don't forwardmetadata_filterBaseKnowledgeSource._save_documents()/ save path — source metadata is never passed to storage when saving chunksCloses #5805
Solution
Close all 4 gaps so metadata flows end-to-end: config → query → search results, and source metadata → saved documents.
Changes
Config layer
lib/crewai/src/crewai/knowledge/knowledge_config.py— addmetadata_filter: dict[str, Any] | Nonefield toKnowledgeConfigQuery layer
lib/crewai/src/crewai/knowledge/knowledge.py— addmetadata_filterparam toquery()andaquery(), forward to storagelib/crewai/src/crewai/crew.py— addmetadata_filterparam toquery_knowledge()andaquery_knowledge(), forward to knowledgeStorage layer
lib/crewai/src/crewai/knowledge/storage/base_knowledge_storage.py— addmetadataparam to abstractsave()andasave()lib/crewai/src/crewai/knowledge/storage/knowledge_storage.py— implement metadata attachment insave()andasave(); attach metadata dict to everyBaseRecordwhen providedSource layer
lib/crewai/src/crewai/knowledge/source/base_knowledge_source.py— passself.metadatatostorage.save()/storage.asave()lib/crewai/src/crewai/knowledge/source/base_file_knowledge_source.py— same for file-based sourcesTests
lib/crewai/tests/knowledge/test_knowledge_metadata.py— 12 unit tests covering all 4 gapsDesign decisions
None, existing code works unchangedmetadatais{}(the default onBaseKnowledgeSource), it's treated asNoneand not attached, keeping records cleanmetadata_filterin KnowledgeConfig — follows the same pattern asresults_limitandscore_threshold, so**knowledge_config.model_dump()works seamlessly withquery()Testing
10 passed, 2 skipped on Windows (async tests skipped due to pytest-recording + asyncio event loop compatibility — CI on Linux covers async paths).
ruff: ✅ all checks passed
mypy: ✅ no issues found in 7 source files