Repository navigation
Conversation
PR TriageMissing issue link: Please link the issue this PR addresses using |
VANDRANKI
left a comment
VANDRANKI
left a comment
There was a problem hiding this comment.
Community review, does not clear the merge gate.
I read the full get_effective_filters function in libs/agno/agno/agent/_utils.py, not just the diff hunk, to check both branches by hand.
Case 1 (agent-level dict + run-level list): knowledge_filters is a list, so execution enters the elif isinstance(knowledge_filters, list): branch. The new isinstance(effective_filters, dict) check is true (agent's filters are the dict), so it now does [*(EQ(k, v) for k, v in effective_filters.items()), *knowledge_filters] - converting the agent's dict entries to EQ objects (agent filters first) before appending the run-level list. Confirmed EQ(key, value) sets self.key/self.value as positional args in agno/filters.py, matching the test's (f.key, f.value) assertions and the expected order [("type","recipe"), ("cuisine","italian"), ("difficulty","easy")].
Case 2 (agent-level list + run-level dict): knowledge_filters is a dict, entering the if isinstance(knowledge_filters, dict): branch. isinstance(effective_filters, dict) is now false (agent's filters are the list), so it takes the new else path: [*effective_filters, *(EQ(k, v) for k, v in knowledge_filters.items())], keeping the agent's list first and appending the run-level dict converted to EQs. This matches the second test's expected order exactly.
Both same-type branches (dict+dict via .update(), list+list via plain concatenation) are untouched by the diff, matching the PR's claim. I checked that the pre-existing list+list concatenation also doesn't do key-based override (it's a plain append, so two EQ objects on the same key both survive as separate AND-ed conditions rather than the later one replacing the earlier one) - that's unchanged behavior, not something this PR makes better or worse, so the docstring's "run-level filters take priority" claim really only fully holds for the dict+dict path where .update() does override by key.
This is a narrowly-scoped, correctly-traced fix for a real bug (the dict-unpacked-into-bare-keys case would have hit Knowledge._validate_filters's "expected FilterExpr instance, got str" and silently dropped the agent's filters, per the PR's cited line numbers). I did not run the test suite myself, but the logic and the two new tests match exactly.
…lter types get_effective_filters unpacked an agent-level dict into its keys when merging with a run-level List[FilterExpr] ([*effective_filters, *knowledge_filters]), dropping the filter values and passing bare strings to knowledge.search(), where _validate_filters drops them for not being FilterExpr instances. The agent's dict filters were silently discarded. Combine mixed filter types into a list of FilterExpr, converting dict entries to EQ expressions so the merged list stays well-formed. The mirror case (agent-level list + run-level dict) now merges the same way instead of replacing the agent filters wholesale, consistent with the dict+dict and list+list merges.
6eb7ab9 to
4a1130c
Compare
…s taken from PR head)
|
Correction to the summary's last bullet: only the dict+dict merge replaces by key, through |
Summary
Agent.knowledge_filtersaccepts adictor aList[FilterExpr], and the run-levelknowledge_filtersargument accepts the same union — so the two levels can legitimately use different types.get_effective_filtersmishandled the merge when they did:[*effective_filters, *knowledge_filters](libs/agno/agno/agent/_utils.py:50onmain) unpacked the dict into its keys, producing a list of bare strings (e.g.["type", EQ("difficulty", "easy")]) and dropping every filter value.update()and list+list concatenation merges.Fixes #10638.
Why
With agent-level
knowledge_filters={"type": "recipe", "cuisine": "italian"}and run-level[EQ("difficulty", "easy")], the merged list contained the bare strings"type"and"cuisine". Downstream,Knowledge._validate_filterslogsInvalid filter at index {i}: expected FilterExpr instance, got strand drops each one (libs/agno/agno/knowledge/knowledge.py:1697-1703), so the agent's filters were silently discarded and retrieval ran without them. Without validation active, the bare strings would reach the vector db adapter, which cannot interpret them either.Changes
libs/agno/agno/agent/_utils.py— when agent-level and run-level filters use different types, convert the dict side toEQexpressions and combine both into a singleList[FilterExpr]. Same-type merges (dict+dict, list+list) are unchanged. The docstring rule "run-level filters taking priority" is preserved: run-level entries are always kept and win.libs/agno/tests/unit/agent/test_run_options.py— regression tests for both mixed-type merges, following the existingTestKnowledgeFilterMergeconventions.Testing
origin/main(pytest tests/unit/agent/test_run_options.py::TestKnowledgeFilterMerge -q): 2 failed —AttributeError: 'str' object has no attribute 'key'(dict unpacked into keys)AssertionError: assert Falsewhere the result was the run-level dict (agent list replaced wholesale)pytest tests/unit/agent/test_run_options.py -q→ 60 passedpytest tests/unit/agent/ -q→ 718 passedorigin/main242 failed, 15685 passed, 499 skipped, 398 errors — identical pre-existing failures (missing optional deps such asgoogleapiclient/redisand docker-backed db tests); the +2 passed are the new tests. No regressions../scripts/format.sh→ all targets pass (2482 files unchanged)./scripts/validate.sh→ ruff check passes for agno/agnoctl/cookbook, pattern check clean; mypy reports 4 errors that also exist on unpatchedorigin/main(os/public/_limits.py:132,db/sqlite/async_sqlite.py:2469,db/postgres/async_postgres.py:2179,db/mysql/async_mysql.py:2096) — none in the changed files.Type of change
Checklist
./scripts/format.shand./scripts/validate.sh)Duplicate and AI-Generated PR Check
Additional Notes
_get_effective_filters(libs/agno/agno/team/_utils.py:41-51) handles the same mixed-type merge differently (it nests the raw dict as the first list element). This PR leaves it untouched; happy to align it in a follow-up if that behavior is also considered wrong.