From b23bf5a0f42fda341523637df6025ddcb643cccd Mon Sep 17 00:00:00 2001 From: Renaud Cepre <32103211+renaudcepre@users.noreply.github.com> Date: Thu, 11 Jun 2026 05:34:03 +0200 Subject: [PATCH 1/2] fix(evals): contains_expected and word_overlap raise when expected is None Both built-ins vacuously passed when ctx.expected_output was None: contains_expected returned True and word_overlap reported a perfect 1.0. Stacked with a case-wiring mistake (expected resolves to None whenever no EvalCase reaches the function), a broken eval looked healthy and the tracked overlap metric was poisoned with fake values. Both now raise ValueError with the case name and a pointer to the fix (set expected, or attach the evaluator per-case when the case set is mixed). The error surfaces as a case error through the existing FixtureError path. Closes #118 --- docs/evals.md | 6 ++++++ protest/evals/evaluators.py | 29 +++++++++++++++++++++++++---- tests/evals/test_e2e.py | 11 +++++++++++ 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/docs/evals.md b/docs/evals.md index 6a68368..24e05d2 100644 --- a/docs/evals.md +++ b/docs/evals.md @@ -371,6 +371,12 @@ Dataclass fields surface as `.` in scores - e.g. `contains_keywords.recall`, `json_valid.valid` (see [Score Namespacing](#score-namespacing)). +`contains_expected` and `word_overlap` require `expected` on the case and +raise when it is `None`: with nothing to compare against, a vacuous pass +(or a fake `overlap=1.0`) would make a case-wiring mistake look like a +healthy run. Attach them only to cases that carry `expected`, per-case if +the case set is mixed. + ## Fixtures Evals use the same fixture system as tests. Expensive setup (database, pipeline, graph) runs once and is shared across all cases. diff --git a/protest/evals/evaluators.py b/protest/evals/evaluators.py index 4e99876..92d80bb 100644 --- a/protest/evals/evaluators.py +++ b/protest/evals/evaluators.py @@ -67,9 +67,19 @@ def contains_keywords( @evaluator def contains_expected(ctx: EvalContext[Any, str], case_sensitive: bool = False) -> bool: - """Check that the output contains expected_output as a substring.""" + """Check that the output contains expected_output as a substring. + + Requires `expected` on the case: with None it has nothing to check, + and passing vacuously would make a wiring mistake (case data not + reaching the eval) look like a healthy run. + """ if ctx.expected_output is None: - return True + raise ValueError( + f"contains_expected on case '{ctx.name}': expected_output is None. " + f"This evaluator needs EvalCase(expected=...) to have something to " + f"check — a vacuous pass would hide a case-wiring mistake. Remove " + f"the evaluator from cases without expected, or set expected." + ) if case_sensitive: return ctx.expected_output in ctx.output return ctx.expected_output.lower() in ctx.output.lower() @@ -147,9 +157,20 @@ def json_valid( @evaluator def word_overlap(ctx: EvalContext[Any, str]) -> WordOverlapResult: - """Compute word overlap between output and expected_output (tracking-only).""" + """Compute word overlap between output and expected_output (tracking-only). + + Requires `expected` on the case: with None there is nothing to overlap + with, and reporting a perfect 1.0 would poison the tracked metric while + hiding a case-wiring mistake. + """ if ctx.expected_output is None: - return WordOverlapResult(overlap=1.0) + raise ValueError( + f"word_overlap on case '{ctx.name}': expected_output is None. " + f"This evaluator needs EvalCase(expected=...) to have something " + f"to compare against — a fake 1.0 would poison the tracked " + f"metric. Remove the evaluator from cases without expected, or " + f"set expected." + ) expected = str(ctx.expected_output) expected_words = set(expected.lower().split()) output_words = set(ctx.output.lower().split()) diff --git a/tests/evals/test_e2e.py b/tests/evals/test_e2e.py index 1cc7159..a621a23 100644 --- a/tests/evals/test_e2e.py +++ b/tests/evals/test_e2e.py @@ -761,6 +761,17 @@ def test_contains_expected(self) -> None: assert e.run(self._make_ctx("Hello World", "world")) is True assert e.run(self._make_ctx("Hello", "world")) is False + def test_contains_expected_none_expected_raises(self) -> None: + """Regression (#118): None expected used to pass vacuously, hiding + case-wiring mistakes.""" + with pytest.raises(ValueError, match="expected_output is None"): + contains_expected.run(self._make_ctx("Hello")) + + def test_word_overlap_none_expected_raises(self) -> None: + """Regression (#118): None expected used to report a fake 1.0.""" + with pytest.raises(ValueError, match="expected_output is None"): + word_overlap.run(self._make_ctx("Hello")) + def test_does_not_contain(self) -> None: e = does_not_contain(forbidden=["cat", "dog"]) assert e.run(self._make_ctx("Yorkshire")).ok is True From f7029b89597ba1349a199ed6abea36f2046cd812 Mon Sep 17 00:00:00 2001 From: Renaud Cepre <32103211+renaudcepre@users.noreply.github.com> Date: Thu, 11 Jun 2026 06:23:36 +0200 Subject: [PATCH 2/2] fix(evals): review follow-ups on the None-expected guard - Error messages now name the per-case attachment mechanism (EvalCase(evaluators=[...])) as the remedy for mixed case sets, and drop the em dashes (repo convention). - Test assertions pin the evaluator name in the match, and the word_overlap test moves next to its positive-path sibling. --- protest/evals/evaluators.py | 12 +++++++----- tests/evals/test_e2e.py | 16 ++++++++++------ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/protest/evals/evaluators.py b/protest/evals/evaluators.py index 92d80bb..e98e6f4 100644 --- a/protest/evals/evaluators.py +++ b/protest/evals/evaluators.py @@ -77,8 +77,9 @@ def contains_expected(ctx: EvalContext[Any, str], case_sensitive: bool = False) raise ValueError( f"contains_expected on case '{ctx.name}': expected_output is None. " f"This evaluator needs EvalCase(expected=...) to have something to " - f"check — a vacuous pass would hide a case-wiring mistake. Remove " - f"the evaluator from cases without expected, or set expected." + f"check; a vacuous pass would hide a case-wiring mistake. Set " + f"expected on the case, or attach this evaluator per-case via " + f"EvalCase(evaluators=[...]) when only some cases carry expected." ) if case_sensitive: return ctx.expected_output in ctx.output @@ -167,9 +168,10 @@ def word_overlap(ctx: EvalContext[Any, str]) -> WordOverlapResult: raise ValueError( f"word_overlap on case '{ctx.name}': expected_output is None. " f"This evaluator needs EvalCase(expected=...) to have something " - f"to compare against — a fake 1.0 would poison the tracked " - f"metric. Remove the evaluator from cases without expected, or " - f"set expected." + f"to compare against; a fake 1.0 would poison the tracked " + f"metric. Set expected on the case, or attach this evaluator " + f"per-case via EvalCase(evaluators=[...]) when only some cases " + f"carry expected." ) expected = str(ctx.expected_output) expected_words = set(expected.lower().split()) diff --git a/tests/evals/test_e2e.py b/tests/evals/test_e2e.py index a621a23..a1db08f 100644 --- a/tests/evals/test_e2e.py +++ b/tests/evals/test_e2e.py @@ -764,14 +764,11 @@ def test_contains_expected(self) -> None: def test_contains_expected_none_expected_raises(self) -> None: """Regression (#118): None expected used to pass vacuously, hiding case-wiring mistakes.""" - with pytest.raises(ValueError, match="expected_output is None"): + with pytest.raises( + ValueError, match=r"contains_expected .* expected_output is None" + ): contains_expected.run(self._make_ctx("Hello")) - def test_word_overlap_none_expected_raises(self) -> None: - """Regression (#118): None expected used to report a fake 1.0.""" - with pytest.raises(ValueError, match="expected_output is None"): - word_overlap.run(self._make_ctx("Hello")) - def test_does_not_contain(self) -> None: e = does_not_contain(forbidden=["cat", "dog"]) assert e.run(self._make_ctx("Yorkshire")).ok is True @@ -850,6 +847,13 @@ def test_word_overlap(self) -> None: assert e.run(self._make_ctx("hello there", "hello world")).overlap == 0.5 assert e.run(self._make_ctx("foo", "hello world")).overlap == 0.0 + def test_word_overlap_none_expected_raises(self) -> None: + """Regression (#118): None expected used to report a fake 1.0.""" + with pytest.raises( + ValueError, match=r"word_overlap .* expected_output is None" + ): + word_overlap.run(self._make_ctx("Hello")) + # --------------------------------------------------------------------------- # Scoring v2: bool verdict, tracking-only metrics