Skip to content

Commit 5aeebb1

Browse files
committed
fix(evals): guard registry membership checks against malformed expected_tools and one-shot iterables
1 parent 7ff3893 commit 5aeebb1

2 files changed

Lines changed: 44 additions & 5 deletions

File tree

evals/agent_trajectory/metrics.py

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,6 @@ def load_golden_samples(
273273
if not isinstance(data, list):
274274
raise ValueError(f"golden samples file must contain a JSON list, got {type(data).__name__}")
275275

276-
known = set(known_tool_names) if known_tool_names is not None else None
277276
golden_fields = {f.name for f in fields(GoldenSample)}
278277
samples: List[GoldenSample] = []
279278
seen_ids: set = set()
@@ -287,7 +286,7 @@ def load_golden_samples(
287286
if sample.id in seen_ids:
288287
raise ValueError(f"duplicate sample id: {sample.id}")
289288
seen_ids.add(sample.id)
290-
issues = validate_golden_sample(sample, known)
289+
issues = validate_golden_sample(sample, known_tool_names)
291290
if issues:
292291
raise ValueError(f"sample '{sample.id}': " + "; ".join(issues))
293292
samples.append(sample)
@@ -302,7 +301,9 @@ def validate_golden_sample(
302301
303302
When ``known_tool_names`` is provided, ``expected_tools`` must be a subset
304303
of it; the caller supplies the authoritative registry names (this module
305-
deliberately does not import ``src/``).
304+
deliberately does not import ``src/``). Any ``Iterable[str]`` is accepted
305+
— including one-shot generators — and materialized once internally, so
306+
membership checks never consume the caller's iterable.
306307
307308
Field *types* are part of the structural contract — hand-edited golden
308309
JSON must fail with a clear message instead of crashing or silently
@@ -311,6 +312,7 @@ def validate_golden_sample(
311312
boolean.
312313
"""
313314
issues: List[str] = []
315+
known = set(known_tool_names) if known_tool_names is not None else None
314316
if not isinstance(sample.id, str) or not sample.id.strip():
315317
issues.append("id must be a non-empty string")
316318
if not isinstance(sample.task_description, str) or not sample.task_description.strip():
@@ -323,8 +325,10 @@ def validate_golden_sample(
323325
issues.append("expected_tools must be a non-empty list")
324326
elif any(not isinstance(t, str) or not t.strip() for t in sample.expected_tools):
325327
issues.append("expected_tools must contain only non-empty strings")
326-
if known_tool_names is not None:
327-
unknown = [t for t in sample.expected_tools if isinstance(t, str) and t.strip() and t not in known_tool_names]
328+
elif known is not None:
329+
# Only reachable when expected_tools is a non-empty list of non-empty
330+
# strings, so malformed values can never crash the membership check.
331+
unknown = [t for t in sample.expected_tools if t not in known]
328332
if unknown:
329333
issues.append(f"unknown expected_tools: {', '.join(unknown)}")
330334
if not isinstance(sample.skills, list):

tests/test_agent_trajectory_metrics.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,28 @@ def test_mistyped_fields_fail_validation_not_crash(self):
428428
assert any("allowed_max_steps must be an integer" in i for i in issues)
429429
assert any("allow_optional_tools must be a boolean" in i for i in issues)
430430

431+
def test_non_iterable_expected_tools_with_known_names_does_not_crash(self):
432+
sample = GoldenSample(
433+
id="x",
434+
task_description="t",
435+
stock_code="600519",
436+
expected_tools=1,
437+
)
438+
issues = validate_golden_sample(sample, {"get_realtime_quote"})
439+
assert any("expected_tools must be a list" in i for i in issues)
440+
441+
def test_registry_membership_with_one_shot_generator(self):
442+
# The helper accepts any Iterable[str]; a one-shot generator must be
443+
# materialized internally so membership checks never consume it.
444+
sample = GoldenSample(
445+
id="x",
446+
task_description="t",
447+
stock_code="600519",
448+
expected_tools=["b", "a"],
449+
)
450+
known = (name for name in ["a", "b"])
451+
assert validate_golden_sample(sample, known) == []
452+
431453

432454
class TestLoadGoldenSamplesErrors:
433455
@staticmethod
@@ -517,6 +539,19 @@ def test_string_expected_tools_raises(self, tmp_path):
517539
with pytest.raises(ValueError, match="expected_tools must be a list"):
518540
load_golden_samples(path=path)
519541

542+
def test_non_list_expected_tools_with_known_names_raises_valueerror(self, tmp_path):
543+
# Regression for OR-COR-4e0e3cf1: the registry membership check must
544+
# not iterate a rejected non-list value and leak a TypeError.
545+
sample = {
546+
"id": "x",
547+
"task_description": "t",
548+
"stock_code": "600519",
549+
"expected_tools": 1,
550+
}
551+
path = self._write_sample(tmp_path, [sample])
552+
with pytest.raises(ValueError, match="expected_tools must be a list"):
553+
load_golden_samples(path=path, known_tool_names={"get_realtime_quote"})
554+
520555
def test_non_bool_allow_optional_tools_raises(self, tmp_path):
521556
sample = {
522557
"id": "x",

0 commit comments

Comments
 (0)