fix(evals): materialize registry once per load, validate ids before dedupe, document summary identity limits

Author: subaoyan16

evals/agent_trajectory/metrics.py

@@ -10,10 +10,13 @@
   Codex App Server backend emits ``arguments_summary`` instead of
   ``arguments``).
 
-All functions in this module are pure: they consume plain data (a log list and
-a ``GoldenSample``) and never touch the filesystem, network, or LLM.  This keeps
-the metrics layer deterministic, unit-testable without an API key, and free of
-``src/`` imports — it can score trajectories from any source.
+The scoring functions in this module are pure: ``compute_trajectory_metrics``,
+``format_text_report`` and ``validate_golden_sample`` consume plain data (a log
+list and a ``GoldenSample``) and never touch the filesystem, network, or LLM.
+The one exception is the loader ``load_golden_samples``, which reads the golden
+JSON file from disk.  This keeps the metrics layer deterministic, unit-testable
+without an API key, and free of ``src/`` imports — it can score trajectories
+from any source.
 
 Idempotency key contract
 ------------------------
@@ -25,6 +28,13 @@
 insertion order or collection type would then change call identity and corrupt
 redundancy / retry counts.
 
+Codex App Server entries carry only ``arguments_summary`` — the redacted and
+truncated preview produced by ``redact_diagnostic_value`` — so their identity
+is best-effort: distinct calls whose summaries collide after redaction or
+truncation may be over-counted as redundant.  A stable argument fingerprint
+requires a producer-side change in ``src/agent/codex_agent_backend.py``, which
+is out of scope for this metrics layer.
+
 Metric semantics
 ----------------
 * ``redundant_calls``: every occurrence of a (tool, args-key) pair beyond its
@@ -273,6 +283,9 @@ def load_golden_samples(
     if not isinstance(data, list):
         raise ValueError(f"golden samples file must contain a JSON list, got {type(data).__name__}")
 
+    # Materialize once before the loop: a one-shot generator must survive the
+    # validation of every sample, not just the first.
+    known = set(known_tool_names) if known_tool_names is not None else None
     golden_fields = {f.name for f in fields(GoldenSample)}
     samples: List[GoldenSample] = []
     seen_ids: set = set()
@@ -283,12 +296,15 @@ def load_golden_samples(
             sample = GoldenSample(**{k: v for k, v in item.items() if k in golden_fields})
         except TypeError as exc:
             raise ValueError(f"sample #{index} has invalid fields: {exc}") from exc
+        # Structural validation runs before duplicate detection so that a
+        # mistyped (possibly unhashable) id is rejected as a ValueError here
+        # instead of crashing the membership check below.
+        issues = validate_golden_sample(sample, known)
+        if issues:
+            raise ValueError(f"sample '{sample.id}': " + "; ".join(issues))
         if sample.id in seen_ids:
             raise ValueError(f"duplicate sample id: {sample.id}")
         seen_ids.add(sample.id)
-        issues = validate_golden_sample(sample, known_tool_names)
-        if issues:
-            raise ValueError(f"sample '{sample.id}': " + "; ".join(issues))
         samples.append(sample)
     return samples
 

tests/test_agent_trajectory_metrics.py

@@ -450,6 +450,12 @@ def test_registry_membership_with_one_shot_generator(self):
         known = (name for name in ["a", "b"])
         assert validate_golden_sample(sample, known) == []
 
+    def test_loader_materializes_registry_once_for_multiple_samples(self):
+        # A one-shot generator must survive loading the whole checked-in file:
+        # the first sample must not exhaust it for the remaining samples.
+        samples = load_golden_samples(known_tool_names=(name for name in _repo_tool_names()))
+        assert len(samples) == 3
+
 
 class TestLoadGoldenSamplesErrors:
     @staticmethod
@@ -552,6 +558,20 @@ def test_non_list_expected_tools_with_known_names_raises_valueerror(self, tmp_pa
         with pytest.raises(ValueError, match="expected_tools must be a list"):
             load_golden_samples(path=path, known_tool_names={"get_realtime_quote"})
 
+    def test_unhashable_id_raises_valueerror(self, tmp_path):
+        # Structural validation must run before duplicate detection: an
+        # unhashable id would otherwise crash the seen_ids membership check
+        # with a TypeError instead of the documented ValueError.
+        sample = {
+            "id": ["x"],
+            "task_description": "t",
+            "stock_code": "600519",
+            "expected_tools": ["get_realtime_quote"],
+        }
+        path = self._write_sample(tmp_path, [sample])
+        with pytest.raises(ValueError, match="id must be a non-empty string"):
+            load_golden_samples(path=path)
+
     def test_non_bool_allow_optional_tools_raises(self, tmp_path):
         sample = {
             "id": "x",