From dfc632aac811365106c69b3f775db78e0da3531a Mon Sep 17 00:00:00 2001 From: Gaurav Gandhi Date: Tue, 11 Aug 2026 21:01:29 +0530 Subject: [PATCH] fix(evaluation): NOT_EVALUATED metric no longer masked by a passing one _generate_final_eval_status looped over an eval case's per-metric results and set the final status to PASSED whenever a PASSED result was seen, using a bare `continue` on NOT_EVALUATED that left an already-set PASSED untouched. A metric that crashed mid-evaluation (caught in _evaluate_metric_for_eval_case and recorded as NOT_EVALUATED) therefore had no effect on the final verdict as long as some other metric in the same eval case passed: [NOT_EVALUATED, PASSED] and [PASSED, NOT_EVALUATED] both reported PASSED, silently dropping the fact that one of the requested metrics never actually produced a verdict. This is order-dependent in a way that has no principled justification -- the two orderings represent the same set of per-metric outcomes and must produce the same final status. FAILED already dominated NOT_EVALUATED correctly in both orderings (it breaks the loop immediately), which is what confirms this was specifically a PASSED-vs-NOT_EVALUATED bug rather than intended behavior. Fix uses the existing EvalStatus values only: if any metric was NOT_EVALUATED and no metric FAILED, the final status is NOT_EVALUATED rather than PASSED. A genuine FAILED still takes precedence over NOT_EVALUATED, since it's real evidence rather than a missing verdict. Added test_generate_final_eval_status_not_evaluated_then_passed_is_not_evaluated and the reverse-order counterpart to make the order-dependence visible, plus a same-precedence test confirming FAILED still wins over NOT_EVALUATED regardless of order. All four generate_final_eval_status tests, including the pre-existing doesn_t_throw_on one, and the full evaluation/ suite (786 tests) pass after the fix. --- .../adk/evaluation/local_eval_service.py | 9 ++ .../evaluation/test_local_eval_service.py | 90 +++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/src/google/adk/evaluation/local_eval_service.py b/src/google/adk/evaluation/local_eval_service.py index 16dcc514c88..bc025515ae6 100644 --- a/src/google/adk/evaluation/local_eval_service.py +++ b/src/google/adk/evaluation/local_eval_service.py @@ -495,6 +495,7 @@ def _generate_final_eval_status( self, overall_eval_metric_results: list[EvalMetricResult] ) -> EvalStatus: final_eval_status = EvalStatus.NOT_EVALUATED + has_not_evaluated_metric = False # Go over all the eval statuses and mark the final eval status as # passed if all of them pass; otherwise, mark the final eval status to # failed. @@ -503,6 +504,7 @@ def _generate_final_eval_status( if overall_eval_status == EvalStatus.PASSED: final_eval_status = EvalStatus.PASSED elif overall_eval_status == EvalStatus.NOT_EVALUATED: + has_not_evaluated_metric = True continue elif overall_eval_status == EvalStatus.FAILED: final_eval_status = EvalStatus.FAILED @@ -510,6 +512,13 @@ def _generate_final_eval_status( else: raise ValueError(f"Unknown eval status: {overall_eval_status}.") + # A metric that never produced a verdict (e.g. a judge call that crashed) + # must not be silently masked by another metric that happened to pass -- + # only report PASSED when every requested metric was actually evaluated. + # A genuine FAILED is real evidence and still takes precedence. + if has_not_evaluated_metric and final_eval_status == EvalStatus.PASSED: + final_eval_status = EvalStatus.NOT_EVALUATED + return final_eval_status async def _perform_inference_single_eval_item( diff --git a/tests/unittests/evaluation/test_local_eval_service.py b/tests/unittests/evaluation/test_local_eval_service.py index 8223adc341d..291e32d1584 100644 --- a/tests/unittests/evaluation/test_local_eval_service.py +++ b/tests/unittests/evaluation/test_local_eval_service.py @@ -629,6 +629,96 @@ def test_generate_final_eval_status_doesn_t_throw_on(eval_service): eval_service._generate_final_eval_status([eval_metric_result]) +def test_generate_final_eval_status_not_evaluated_then_passed_is_not_evaluated( + eval_service, +): + """A metric that never produced a verdict must not be masked by a later PASSED. + + If metric_1 crashed (NOT_EVALUATED) and metric_2 passed, the eval case did + not actually pass every requested metric -- it should be reported as + NOT_EVALUATED, not PASSED. + """ + results = [ + EvalMetricResult( + metric_name="metric1", + threshold=0.5, + eval_status=EvalStatus.NOT_EVALUATED, + ), + EvalMetricResult( + metric_name="metric2", threshold=0.5, eval_status=EvalStatus.PASSED + ), + ] + + assert ( + eval_service._generate_final_eval_status(results) + == EvalStatus.NOT_EVALUATED + ) + + +def test_generate_final_eval_status_passed_then_not_evaluated_is_not_evaluated( + eval_service, +): + """Same as above with the metrics in the opposite order. + + Both orderings of [PASSED, NOT_EVALUATED] must produce the same final + status. Prior to this fix they didn't: this ordering silently returned + PASSED, which is exactly the evidence that the old behavior was a bug + and not intended. + """ + results = [ + EvalMetricResult( + metric_name="metric1", threshold=0.5, eval_status=EvalStatus.PASSED + ), + EvalMetricResult( + metric_name="metric2", + threshold=0.5, + eval_status=EvalStatus.NOT_EVALUATED, + ), + ] + + assert ( + eval_service._generate_final_eval_status(results) + == EvalStatus.NOT_EVALUATED + ) + + +def test_generate_final_eval_status_failed_dominates_not_evaluated( + eval_service, +): + """A genuine FAILED is real evidence and must still win over NOT_EVALUATED, + regardless of order -- only the PASSED-vs-NOT_EVALUATED case was buggy. + """ + failed_then_not_evaluated = [ + EvalMetricResult( + metric_name="metric1", threshold=0.5, eval_status=EvalStatus.FAILED + ), + EvalMetricResult( + metric_name="metric2", + threshold=0.5, + eval_status=EvalStatus.NOT_EVALUATED, + ), + ] + not_evaluated_then_failed = [ + EvalMetricResult( + metric_name="metric1", + threshold=0.5, + eval_status=EvalStatus.NOT_EVALUATED, + ), + EvalMetricResult( + metric_name="metric2", threshold=0.5, eval_status=EvalStatus.FAILED + ), + ] + + assert ( + eval_service._generate_final_eval_status(failed_then_not_evaluated) + == EvalStatus.FAILED + ) + assert ( + eval_service._generate_final_eval_status(not_evaluated_then_failed) + == EvalStatus.FAILED + ) + + @pytest.mark.asyncio async def test_mcp_stdio_agent_no_runtime_error(mocker): """Test that LocalEvalService can handle MCP stdio agents without RuntimeError.