Skip to content

Commit 3f372ff

Browse files
riolocclaude
andcommitted
fix: address CodeRabbit review comments on agents config layer
- Avoid input dict mutation in AgentsConfig.extract_agent_definitions by shallow-copying before pop - Use agents.update(value) instead of agents = value to merge rather than overwrite collected agent definitions - Add mcp_headers field to HttpApiAgentConfig so api-to-agents migration preserves MCP headers instead of silently dropping them - Remove field filtering in migrate_api_to_agents since all APIConfig fields now have corresponding HttpApiAgentConfig fields - Add field_validator on EvaluationData.agent to reject whitespace-only agent names Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent c33d395 commit 3f372ff

6 files changed

Lines changed: 69 additions & 10 deletions

File tree

src/lightspeed_evaluation/core/models/agents.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@ class HttpApiAgentConfig(BaseModel):
6161
ge=0,
6262
description="Maximum number of retry attempts for 429 errors",
6363
)
64+
mcp_headers: Optional[dict[str, Any]] = Field(
65+
default=None,
66+
description="MCP headers configuration for authentication",
67+
)
6468

6569
@field_validator("endpoint_type")
6670
@classmethod
@@ -112,13 +116,14 @@ def extract_agent_definitions(cls, data: Any) -> Any:
112116
if not isinstance(data, dict):
113117
return data
114118

119+
data = dict(data)
115120
agents: dict[str, Any] = {}
116121
default_data = data.pop("default", {})
117122
remaining: dict[str, Any] = {}
118123

119124
for key, value in data.items():
120125
if key == "agents":
121-
agents = value
126+
agents.update(value)
122127
elif isinstance(value, dict) and "type" in value:
123128
agents[key] = value
124129
else:

src/lightspeed_evaluation/core/models/data.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,14 @@ def is_metric_invalid(self, metric: str) -> bool:
423423
"""Returns True if the metric didn't pass the validation."""
424424
return metric in self._invalid_metrics
425425

426+
@field_validator("agent")
427+
@classmethod
428+
def validate_agent_not_whitespace(cls, v: Optional[str]) -> Optional[str]:
429+
"""Validate agent name is not whitespace-only."""
430+
if v is not None and not v.strip():
431+
raise ValueError("agent name cannot be empty or whitespace")
432+
return v
433+
426434
@field_validator("conversation_metrics")
427435
@classmethod
428436
def validate_conversation_metrics(

src/lightspeed_evaluation/core/models/system.py

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
SUPPORTED_ENDPOINT_TYPES,
4040
SUPPORTED_GRAPH_TYPES,
4141
)
42-
from lightspeed_evaluation.core.models.agents import AgentsConfig, HttpApiAgentConfig
42+
from lightspeed_evaluation.core.models.agents import AgentsConfig
4343
from lightspeed_evaluation.core.storage.config import StorageBackendConfig
4444
from lightspeed_evaluation.core.system.exceptions import ConfigurationError
4545

@@ -874,18 +874,12 @@ def migrate_api_to_agents(cls, data: Any) -> Any:
874874
return data
875875

876876
api_data = data["api"]
877-
valid_fields = set(HttpApiAgentConfig.model_fields.keys())
878877
if isinstance(api_data, dict):
879878
api_enabled = api_data.get("enabled", True)
880-
agent_fields = {
881-
k: v
882-
for k, v in api_data.items()
883-
if k != "enabled" and k in valid_fields
884-
}
879+
agent_fields = {k: v for k, v in api_data.items() if k != "enabled"}
885880
elif isinstance(api_data, BaseModel):
886881
api_enabled = getattr(api_data, "enabled", True)
887-
dumped = api_data.model_dump(exclude={"enabled"})
888-
agent_fields = {k: v for k, v in dumped.items() if k in valid_fields}
882+
agent_fields = api_data.model_dump(exclude={"enabled"})
889883
else:
890884
return data
891885

tests/unit/core/models/test_agents.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,21 @@ def test_num_retries_must_be_non_negative(self) -> None:
6767
with pytest.raises(ValidationError):
6868
HttpApiAgentConfig(num_retries=-1)
6969

70+
def test_mcp_headers_accepted(self) -> None:
71+
"""Test mcp_headers field is accepted."""
72+
config = HttpApiAgentConfig(
73+
mcp_headers={"enabled": True, "servers": {"mcp1": {"env_var": "TOKEN"}}}
74+
)
75+
assert config.mcp_headers == {
76+
"enabled": True,
77+
"servers": {"mcp1": {"env_var": "TOKEN"}},
78+
}
79+
80+
def test_mcp_headers_defaults_to_none(self) -> None:
81+
"""Test mcp_headers defaults to None."""
82+
config = HttpApiAgentConfig()
83+
assert config.mcp_headers is None
84+
7085
def test_extra_forbid(self) -> None:
7186
"""Test unknown fields are rejected."""
7287
with pytest.raises(ValidationError):
@@ -167,6 +182,17 @@ def test_unknown_top_level_key_rejected(self) -> None:
167182
}
168183
)
169184

185+
def test_input_dict_not_mutated(self) -> None:
186+
"""Test that the input dict is not mutated by the model validator."""
187+
input_data = {
188+
"default": {"agent": "ols_api"},
189+
"ols_api": {"type": "http_api"},
190+
}
191+
original_keys = set(input_data.keys())
192+
AgentsConfig.model_validate(input_data)
193+
assert set(input_data.keys()) == original_keys
194+
assert "default" in input_data
195+
170196

171197
class TestAgentsConfigResolve:
172198
"""Tests for AgentsConfig.resolve_agent_config method."""

tests/unit/core/models/test_data.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -687,3 +687,12 @@ def test_empty_agent_name_rejected(self) -> None:
687687
agent="",
688688
turns=[TurnData(turn_id="t1", query="Q")],
689689
)
690+
691+
def test_whitespace_agent_name_rejected(self) -> None:
692+
"""Whitespace-only agent name is rejected."""
693+
with pytest.raises(ValidationError, match="agent name cannot be empty"):
694+
EvaluationData(
695+
conversation_group_id="cg1",
696+
agent=" ",
697+
turns=[TurnData(turn_id="t1", query="Q")],
698+
)

tests/unit/core/models/test_system.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -775,3 +775,20 @@ def test_invalid_default_agent_reference_raises(self) -> None:
775775
agents={"ols_api": HttpApiAgentConfig()},
776776
)
777777
)
778+
779+
def test_mcp_headers_preserved_during_migration(self) -> None:
780+
"""mcp_headers from api: are preserved in the migrated agents config."""
781+
config = SystemConfig.model_validate(
782+
{
783+
"api": {
784+
"enabled": True,
785+
"mcp_headers": {
786+
"enabled": True,
787+
"servers": {},
788+
},
789+
},
790+
}
791+
)
792+
assert config.agents is not None
793+
agent = config.agents.agents["http_api"]
794+
assert agent.mcp_headers == {"enabled": True, "servers": {}}

0 commit comments

Comments
 (0)