Repository navigation
fix(llms): coerce dict tool_search config instead of silently disabling it - #8738
梦想陆地的鱼 (heyseere) wants to merge 2 commits into
Conversation
Strix Security ReviewWarning This pull request has 85 commits after the last Strix review ( ✅ No open security findings. PR summary: This PR fixes Anthropic LLM config handling so a `tool_search` value supplied as a dict (as YAML/JSON agent configs produce) is coerced into `AnthropicToolSearchConfig` by the pydantic field instead of being silently discarded; only `True` (default variant) and `False`/`None` (disabled) remain special-cased. It also adds tests for dict coercion, `False` disabling, and failure on invalid dicts. Review summaryNo security issues found. The change narrows a pre-validator so only Review 1 on this PR · Reviewed by Strix |
7b044b0 to
1d9939b
Compare
|
Friendly ping for a review when you have a moment. All bot checks are green and the PR is mergeable; happy to adjust anything. |
9a249c9 to
5b2d371
Compare
SIDDARTHA REDDY (SIDDARTHAREDDY8)
left a comment
There was a problem hiding this comment.
I read the diff in lib/crewai/src/crewai/llms/providers/anthropic/completion.py (_normalize_anthropic_fields) and the new tests in test_anthropic.py, plus AnthropicToolSearchConfig. I did not run the test suite.
-
test_tool_search_invalid_dict_raises expects ImportError matching "tool_search", but AnthropicToolSearchConfig is a plain Pydantic model with type: Literal["regex", "bm25"], so a dict {"type": "bogus"} should fail validation with a ValidationError, not an ImportError. The only ImportError in completion.py I found is the module-level anthropic SDK import, whose message does not mention tool_search. Can you confirm this test passes for the validation reason, rather than passing or failing for an unrelated import path? If the intent is "fail loudly", asserting the validation error type would pin that down.
-
An empty dict now enables tool search: tool_search={} is not True or False, so it passes through to the field and coerces to AnthropicToolSearchConfig() with the default type "bm25". Before this change it was dropped to None. Is that flip intended? A config carrying tool_search: {} arguably means "present with defaults", but it is a behavior change worth calling out in the PR description.
-
The same pass-through now applies to other non-dict values: a string such as "regex", or an int or list, previously became None and now reaches Pydantic validation and errors. That seems consistent with "fail loudly", but it is broader than the dict case in the PR description. Is erroring on those inputs intended, or should only dicts and config instances be coerced while other types are still dropped?
|
Thanks for the careful read — took all three points back to the code and the test run: 1. ImportError vs ValidationError — the test does pass for the validation reason, but via one hop I should have pointed at: 2. 3. Non-dict values — also intended, same fail-loudly rationale: previously a typo like |
75a2ff1 to
57648ca
Compare
…class Assert the invalid-dict tool_search failure as a pydantic ValidationError on AnthropicCompletion itself, instead of relying on the LLM factory wrapping it into an ImportError whose message happens to embed the pydantic dump (an implementation detail of llm.py, not the contract under test). Also assert the error location is tool_search.type. Add a companion case pinning the factory path via the exception cause chain: LLM(...) raises ImportError with __cause__ set to the ValidationError, replacing the previous fragile message string match. Both tests fail against the pre-PR behavior (invalid dicts silently became None), confirmed by reverting completion.py to 86dca29. From ZCode
57648ca to
f17e9d2
Compare
Related issue
Fixes #8734
Summary
The Anthropic pre-validator discarded any
tool_searchvalue that was notTrueor anAnthropicToolSearchConfiginstance — including plain dicts, which is exactly what YAML/JSON agent configs produce when thellm:block is passed throughLLM(**value). Result:tool_search: {type: regex}silently disabled tool search instead of coercing.This keeps special-casing only
True(default variant) andFalse/None(disabled) and lets the pydantic field (annotatedAnthropicToolSearchConfig | None) coerce and validate dict inputs, so invalid values now fail loudly instead of silently disabling.Two behavior changes beyond the headline dict case, both intended:
tool_search: {}) now enables tool search with the defaultAnthropicToolSearchConfig()(bm25 variant) — previously it was silently dropped toNone. Reading it as "present with defaults" matches the field's coercion semantics.True/False/Nonevalue (e.g. a string, int, or list) now reaches the pydantic field and fails validation at construction — previously it was also silently dropped toNone. Same fail-loudly rationale: a typo liketool_search: regeexshould surface, not silently disable the feature.Verification
lib/crewai/tests/llms/anthropic/test_anthropic.py(114 passed): tests cover dict coercion,tool_search=False→None, and loud failure on an invalid dict — asserted directly asValidationErroron a directly-constructedAnthropicCompletion, with a supplementary case pinning theLLMfactory path (ImportErrorwith theValidationErroras__cause__).ruff checkclean on changed files.Disclosure: this PR was produced with AI assistance (ZCode agent) and human-reviewed — please apply the
llm-generatedlabel.Additional context
None
From ZCode
Note
Low Risk
Narrow change to Anthropic LLM config normalization; improves correctness for agent YAML and makes misconfiguration visible instead of silent.
Overview
Fixes Anthropic
tool_searchhandling when the value comes from YAML/JSON as a dict (e.g.{type: regex}). The pre-validator inAnthropicCompletion._normalize_anthropic_fieldsno longer strips dicts and other non-boolean inputs toNone; it only mapstool_search=Trueto a defaultAnthropicToolSearchConfig()andtool_search=FalsetoNone, leaving everything else for the Pydantic field to coerce or validate.YAML agent configs with
tool_search: {type: regex}now enable tool search instead of silently turning it off. Invalid configs raiseValidationErrorat construction (surfacing asImportErrorvia theLLMfactory), andtool_search=Falseis explicitly tested to clear the feature.Tests in
test_anthropic.pycover dict coercion, explicit disable, and loud failure on invalidtypevalues.Reviewed by Cursor Bugbot for commit f17e9d2. Bugbot is set up for automated code reviews on this repo. Configure here.