From bbcebffbf904fadb561e96dc0c4a2543c78768ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Moura?= Date: Fri, 18 Sep 2026 10:02:33 -0300 Subject: [PATCH] fix(llm_overlay): a role and a key that differ only by surrounding whitespace match (#7572) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(llm_overlay): a role and a key that differ only by surrounding whitespace match A role that comes from a YAML file often ends in a newline — `role: >` folds to "Researcher\n" — and a caller writes the key for the clean text, "Researcher". The two never matched, so the agent kept its declared llm without a word: the overlay looked active and did nothing. `llm_overlay(mapping)` now sets a copy of the mapping with the whitespace around each key dropped, and `overlay_model_for(role)` strips the role before looking it up; an empty or None role matches nothing. Matching is otherwise unchanged: exact text, no case folding. The mapping the caller passed is not touched. The three readers (Agent at construction and after interpolation, LiteAgent at construction) already go through overlay_model_for, so they pick this up with no change of their own. Tests: a key "Researcher" matches "Researcher\n" and " Researcher "; a key written with a trailing newline matches a clean role; case and inner whitespace still miss, as do "" and None; the caller's mapping is not mutated; a YAML-folded template role matches after interpolation. Co-Authored-By: Claude Fable 5.1 * fix(llm_overlay): two keys that are one role with different models are refused Review on #7572 (CodeRabbit, iris-clawd): after stripping, "Researcher" and " Researcher " are one key, and the later entry silently won — the model an agent ran on depended on dictionary order. `_stripped` now refuses a mapping that names one role twice with different models (ValueError naming the role and both models) and keeps a harmless duplicate that names the same model once. A test pins both. Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: Claude Fable 5.1 --- lib/crewai/src/crewai/llm_overlay.py | 45 ++++++++++++++++++--- lib/crewai/tests/test_llm_overlay.py | 59 ++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 6 deletions(-) diff --git a/lib/crewai/src/crewai/llm_overlay.py b/lib/crewai/src/crewai/llm_overlay.py index 1397052c7..8cb520d6a 100644 --- a/lib/crewai/src/crewai/llm_overlay.py +++ b/lib/crewai/src/crewai/llm_overlay.py @@ -6,7 +6,9 @@ an ``Agent`` or ``LiteAgent`` whose ``role`` is a key of the mapping is built with the mapped model instead of its declared ``llm``. Roles that are not keys, and every agent built outside the block, keep their own model. -Roles are matched exactly, by the text they have when the overlay is read. +Roles are matched exactly, by the text they have when the overlay is read; +only whitespace around a role or a key is ignored, so a role a YAML file +leaves with a trailing newline matches a key written for the clean text. An ``Agent`` is read when it is built, with its declared role, and read again when ``crew.kickoff(inputs=...)`` interpolates the inputs into that role and the text changes. The second read is what lets a role declared as @@ -71,22 +73,53 @@ def llm_overlay(mapping: dict[str, str] | None) -> Iterator[None]: Args: mapping: ``{role: model}`` for the block; ``None`` clears any active - overlay for the block. The previous value is always restored on - exit, including when the block raises. + overlay for the block. Whitespace around each role is dropped from + the copy the block uses, so a key matches a role that differs from + it only by its surrounding whitespace; the mapping passed in is + left as it is. The previous value is always restored on exit, + including when the block raises. """ - token = active.set(mapping) + token = active.set(_stripped(mapping)) try: yield finally: active.reset(token) -def overlay_model_for(role: str) -> str | None: +def overlay_model_for(role: str | None) -> str | None: """The model the active overlay assigns to ``role``. + Whitespace around ``role`` is ignored, as it was around the keys when the + overlay was set. An empty or ``None`` role matches nothing. + Returns: The mapped model string, or ``None`` when no overlay is active or ``role`` is not one of its keys. """ mapping = active.get() - return mapping.get(role) if mapping else None + if not mapping or role is None: + return None + role = role.strip() + return mapping.get(role) if role else None + + +def _stripped(mapping: dict[str, str] | None) -> dict[str, str] | None: + """A copy of ``mapping`` with the whitespace around each role dropped. + + Two keys that differ only by whitespace name one role. When they name the + same model the copy holds it once; when they name different models the + mapping is ambiguous and is refused, so the model an agent runs on never + depends on dictionary order. + """ + if mapping is None: + return None + stripped: dict[str, str] = {} + for role, model in mapping.items(): + key = role.strip() + if key in stripped and stripped[key] != model: + raise ValueError( + f"llm_overlay: role {key!r} is mapped twice with different models " + f"({stripped[key]!r} and {model!r}); give each role one model" + ) + stripped[key] = model + return stripped diff --git a/lib/crewai/tests/test_llm_overlay.py b/lib/crewai/tests/test_llm_overlay.py index 9a91a15a7..0e329bd3d 100644 --- a/lib/crewai/tests/test_llm_overlay.py +++ b/lib/crewai/tests/test_llm_overlay.py @@ -85,6 +85,65 @@ def test_nested_overlay_restores_the_outer_one() -> None: assert overlay_model_for("Researcher") == "openai/gpt-4o" +def test_whitespace_around_a_role_or_a_key_is_ignored() -> None: + """A role read from a YAML file often ends in a newline (``role: >`` folds + to one) while the caller writes the key for the clean text; the newline can + just as well land on the key. Either side is stripped, on the direct lookup + and on the read an agent does when it is built.""" + with llm_overlay(OVERLAY): + assert overlay_model_for("Researcher\n") == "openai/gpt-4o" + assert overlay_model_for(" Researcher ") == "openai/gpt-4o" + assert _agent("Researcher\n").llm.model == "gpt-4o" + + with llm_overlay({"Researcher\n": "openai/gpt-4o"}): + assert overlay_model_for("Researcher") == "openai/gpt-4o" + assert _agent("Researcher").llm.model == "gpt-4o" + + +def test_only_the_whitespace_around_the_text_is_forgiven() -> None: + """The text in between is still matched exactly, and nothing matches an + empty role.""" + with llm_overlay(OVERLAY): + assert overlay_model_for("researcher") is None + assert overlay_model_for("Re searcher") is None + assert overlay_model_for("Writer\n") is None + assert overlay_model_for("") is None + assert overlay_model_for(" \n") is None + assert overlay_model_for(None) is None + + +def test_two_keys_that_are_one_role_with_different_models_are_refused() -> None: + """``"Researcher"`` and ``" Researcher "`` name one role. With one model + they collapse to it; with two the mapping is ambiguous and refused, so the + model never depends on dictionary order.""" + with llm_overlay({"Researcher": "openai/gpt-4o", " Researcher ": "openai/gpt-4o"}): + assert overlay_model_for("Researcher") == "openai/gpt-4o" + + with pytest.raises(ValueError, match="'Researcher' is mapped twice with different models"): + with llm_overlay({"Researcher": "openai/gpt-4o", " Researcher ": "openai/gpt-4o-mini"}): + pass # never entered + assert active.get() is None # nothing was set + + +def test_the_mapping_the_caller_passed_is_not_mutated() -> None: + mapping = {"Researcher\n": "openai/gpt-4o"} + with llm_overlay(mapping): + assert active.get() == {"Researcher": "openai/gpt-4o"} + assert mapping == {"Researcher\n": "openai/gpt-4o"} + + +def test_a_yaml_folded_template_role_matches_after_interpolation() -> None: + """The CrewBase shape: a templated role from YAML keeps its trailing newline + through interpolation, and the key is written for the clean text.""" + with llm_overlay(TEMPLATE_OVERLAY): + agent = _agent("Researcher for {repo}\n") + assert agent.llm.model == "gpt-4o-mini" + agent.interpolate_inputs({"repo": "crewAIInc/x"}) + + assert agent.role == "Researcher for crewAIInc/x\n" + assert agent.llm.model == "gpt-4o" + + @pytest.mark.filterwarnings("ignore:LiteAgent is deprecated") def test_lite_agent_gets_the_overlay_model() -> None: with llm_overlay(OVERLAY):